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

Modern Business - FSE - align add block button to right on template editor to stop if conflicting with centred content #1269

Closed
wants to merge 1 commit into from

Conversation

glendaviesnz
Copy link
Contributor

Changes proposed in this Pull Request:

  • Align the add block button to the right and drop down slightly when editing FSE template to prevent it conflicting with block selection now we have reduced block margins

Testing

  • Build the theme and copy to your FSE test environment
  • Edit the header and footer templates, but and check that the Site Title & Site Description blocks in particular are easy to select
  • Also check that the Add Block floating button is still accessible to the right when needed

Before

before-button

After

after-button

Related issue(s):

Part of fix for Automattic/wp-calypso#35421

…ditor to stop if conflicting with centred content
@gwwar
Copy link

gwwar commented Aug 16, 2019

@apeatling @shaunandrews any thoughts on the updated inserter placement? I'm guessing folks will have a hard time interacting with it on the right, esp since we need a hover effect here.

Not sure I'm a huge fan of the hover interaction with the inserter in the first place, but open to ideas on a quick fix here to avoid the accidental insert.

@Copons
Copy link
Contributor

Copons commented Aug 19, 2019

Imho we have some rogue style that messes up the inserter positioning somehow, because it's not supposed to appear that much inside the previous block.

Gutenberg Proper FSE Template
Screenshot 2019-08-19 at 14 35 37 Screenshot 2019-08-19 at 14 39 47
Screenshot 2019-08-19 at 14 35 51 Screenshot 2019-08-19 at 14 37 23

Also note in the little "Image" label in the first FSE screen: it means that the blocks are so close/overlapping that hovering the inserter active area even highlights the following block!

I don't think we should move the inserter, and if we do, we better have a great case for it, because I'm 100% sure folks will be questioning it soon and vocally. 😄

@apeatling
Copy link
Member

I agree, I don't think we should be moving the inserter since it's a core UI and the expectation is that it will be in the middle above the block. I agree with @Copons assessment -- there should be space for the inserter, even if it means blocks are more spaced out. The main thing is to be consistent between editing the header and editing the main page view.

@Copons
Copy link
Contributor

Copons commented Aug 19, 2019

BTW, when I rebased Automattic/wp-calypso#35387, I've also found the wrong margin culprit and removed it.
It's a very simple fix, and it results in only changing the blocks spacing (in the editor) of a few pixels, so much that most of the time I couldn't notice the difference without looking at master and refactor side by side:

https://github.com/Automattic/wp-calypso/pull/35387/files#diff-f8be354e139d906116cfe9237c1c2100L19-L23

@glendaviesnz
Copy link
Contributor Author

glendaviesnz commented Aug 19, 2019

looks like we are not the only ones with floating button issues - p58i-7YJ-p2 #comment-42796 - main ones to note that affects our specific issue are:

WordPress/gutenberg#13571

WordPress/gutenberg#16646

In summary it looks like moves are afoot in core to change placement of this button. I will take a look at @Copons PR first - hopefully that fixes things enough so we can just close this one and wait for changes on this in core.

@glendaviesnz glendaviesnz added [Type] Bug Something isn't working and removed [Type] Bug Something isn't working labels Aug 19, 2019
@glendaviesnz
Copy link
Contributor Author

Closing for now as this does not look like it is the best approach to take.

@glendaviesnz glendaviesnz deleted the fix/fse-add-block-button-alignment branch August 19, 2019 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants