Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

onWriting Flow: Remove the provisional block behavior #8706

Merged
merged 2 commits into from
Aug 8, 2018

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Aug 8, 2018

closes #8678

I admit I never been fan of the provisional block behavior: The fact to remove an empty paragraph right upon its creation if it's unselected though I understood it was useful when it was too easy to create in between paragraphs which is not the case anymore.

I think it's better to remove it to avoid any unwanted side effect it can create like #8678. Thoughts @jasmussen @aduth

Testing instructions

Repeat instructions in #8678 and check that it works properly.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Aug 8, 2018
@youknowriad youknowriad self-assigned this Aug 8, 2018
@youknowriad youknowriad requested a review from a team August 8, 2018 09:43
*
* @return {?string} Provisional block client ID, if set.
*/
export function getProvisionalBlockClientId( state ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitated to keep this selector fo BC but the probability for it to be used outside Gutenberg is 0 IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could keep the function, and have it return null (the default reducer state) so it would at least not throw an error and be as close as possible to a real value with the state no longer existing.

But I'd probably agree it's unlikely that this is used.

@youknowriad youknowriad force-pushed the remove/provisional-block branch from b0e0632 to faeab9d Compare August 8, 2018 10:53
@youknowriad youknowriad force-pushed the remove/provisional-block branch from faeab9d to 3c05051 Compare August 8, 2018 14:10
@youknowriad
Copy link
Contributor Author

I want my badge @tofumatt 😄

@aduth aduth self-requested a review August 8, 2018 14:28
@youknowriad youknowriad added this to the 3.5 milestone Aug 8, 2018
@aduth
Copy link
Member

aduth commented Aug 8, 2018

Thoughts ahead of my review: I agree this has added complexity and therefore caused fragility. It's interesting to consider this as the cause for #8678.

Usage-wise, I'm trying to remember back to #5417 / #5396 and the issues this was meant to address:

  • The inserter line was certainly easier to hit at the time (it spanned the full content width and was quite large).
  • I guess we don't show the sibling inserter if there's already a blank new paragraph between two other blocks? I recall one of the issues being that you could easily add many empty paragraphs and the placeholders would be visually ugly.
  • In retrospect, the auto-deletion seems quite extreme, though it is a bit strange to see the writing placeholder "Add text or type / to add content" when clicking sibling inserter and then to another block; perhaps the best avenue here is to limit the effect of the placeholder. Or maybe it's completely fine as it is.

@aduth
Copy link
Member

aduth commented Aug 8, 2018

There's still one instance of the phrase "provisional" in the codebase in writing-flow.test.js. Unsure if we want to abandon the terminology altogether. In the test, I believe it's more accurately referring to an unmodified default block.

// When returning to Visual mode, backspace in selected block should
// reset to the provisional block.
await page.keyboard.press( 'Backspace' );

*
* @return {?string} Provisional block client ID, if set.
*/
export function getProvisionalBlockClientId( state ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we could keep the function, and have it return null (the default reducer state) so it would at least not throw an error and be as close as possible to a real value with the state no longer existing.

But I'd probably agree it's unlikely that this is used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing Flow: Block merges twice on empty intermediary paragraphs
2 participants