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

only show Add Block Here indicator when block is not replaceable #445

Merged
merged 2 commits into from
Dec 27, 2018

Conversation

mzorz
Copy link
Contributor

@mzorz mzorz commented Dec 24, 2018

Fix #439

This PR checks whether the selected block is replaceable and only shows the ADD BLOCK HERE when it is not going to be replaced.

Also, we're using the i18n API now for the ADD BLOCK HERE string.

addblockhere

@mzorz mzorz added the [Type] Enhancement Improves a current area of the editor label Dec 24, 2018
@mzorz mzorz added this to the Beta milestone Dec 24, 2018
@mzorz mzorz requested a review from etoledom December 24, 2018 13:33
@iamthomasbishop
Copy link
Contributor

I wouldn't do this – The inserter trigger should always act the same (insert a new block) regardless of what block type is focused. We should probably avoid block replace/transforms in general until we have the transform flow built into the toolbar.

Copy link
Contributor

@etoledom etoledom left a comment

Choose a reason for hiding this comment

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

Hey @mzorz , this is working perfect!

Let's wait to see what do we decide to do with this behavior in general, but this PR I'd say is ready to merge any time ✅

@koke
Copy link
Member

koke commented Dec 26, 2018

The thing here is that we haven't planned to do block transformations for the beta. I understand how it might seem confusing in some scenarios, but the lack of it would also be confusing in others without block transformations:

  • Write a paragraph
  • You want to add an image so instinctively tap enter to break away from the paragraph
  • Insert an image

Without this logic, the image would be added after the new empty paragraph, and you would have to remove it manually.

BTW, right now the only thing that is replaced is an empty paragraph, which is the default block. I see the web version also replaces empty headings, but I think that's actually done by block transformations.

@mzorz
Copy link
Contributor Author

mzorz commented Dec 26, 2018

I think this PR just makes things consistent with the introduction of "add block here" label in #124 by following the original inserter designs here:

gutenberg - inserter v1 - android

Also it's important to note that the replacement behavior was introduced in #327 and slightly modified in #338 with the reasoning behind it being to match the web's behavior at that moment. This PR in particular is a fix to make things consistent with all these 3 things.

Given it's totally valid we either change our minds and/or Gutenberg changes behavior as well, I think we should merge this particular PR and bring the discussion of transformations and their utility within the scope of "inserting when on an empty block" on another issue with designs that can be followed up on different PRs. Wdyt @koke @iamthomasbishop ?

@mzorz
Copy link
Contributor Author

mzorz commented Dec 26, 2018

Thank you for the review @etoledom ! 🙇 ❤️

@iamthomasbishop
Copy link
Contributor

I still think the behavior is slightly confusing and think we should be intentional about keeping the two things (insert, replace/transform) separate, but for now, how about we just remove the sheet title so the user doesn't have to worry about the distinction here?

I think eventually, we need to show the block type icon in the toolbar to indicate what type of block the user is on, but this should ease the concern for now.

@hypest
Copy link
Contributor

hypest commented Dec 27, 2018

I think that the "Replace block" title is not proper. Gutenberg-web doesn't have that notion. The closest notion is "Transformations", but that's not the feature we're implementing here at the moment.

I also think that the Inserter we have on mobile, is matching the contextual inserter button from the web, not the global one. The contextual one always "replaces" the empty block placeholder instead of appending a new block. It's title though still is "Add block". The default block placeholder should not be considered a block though... it's only a fancy/elaborate indicator.

Long story short, I think we should work towards the proper behavior anyway, which I think is:

  • Always show "Add block" in the Inserter's title
  • Introduce the proper "default block placeholder", which is not the paragraph block

None of the above need to be done in this PR though. What the current PR only does is to remove the stray "ADD BLOCK HERE" indicator that currently appears (see #439). I think it should be OK to merge it and also open a new ticket to lose the "REPLACE BLOCK" title case and another ticket to introduce the proper default block placeholder.

@koke
Copy link
Member

koke commented Dec 27, 2018

another ticket to introduce the proper default block placeholder.

That discussion is going on in #449

@mzorz
Copy link
Contributor Author

mzorz commented Dec 27, 2018

Merging this one and letting the conversation continue until a decision is made

@mzorz mzorz merged commit 79d178f into develop Dec 27, 2018
@mzorz mzorz deleted the issue/439-remove-add-block-here-default branch December 27, 2018 11:47
@mzorz
Copy link
Contributor Author

mzorz commented Dec 27, 2018

think it should be OK to merge it and also open a new ticket to lose the "REPLACE BLOCK" title case and another ticket to introduce the proper default block placeholder.

opened new ticket in #452 and #453 as per this ☝️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants