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

Block: fix invalid block scrim overflowing toolbar on mobile #9473

Merged
merged 5 commits into from Sep 13, 2018

Conversation

Projects
None yet
5 participants
@johngodley
Contributor

johngodley commented Aug 30, 2018

On small screens the invalid block scrim overflows a block’s dropdown menu / toolbar, which is positioned beneath the block.

edit_post_ wordpress_latest _wordpress

This only happens when the block is selected.

This PR shortens the scrim so it doesn't overlap the bottom toolbar, as discussed in #9402.

Fixes #9402

Testing

  1. Make a block invalid (i.e. edit HTML on a paragraph and remove the trailing </p>)
  2. Switch browser to a mobile display
  3. Ensure block is selected and verify that without this PR the scrim overflows the bottom of the block, as shown above
  4. Apply PR and verify that the scrim does not cover the toolbar and dropdown menu at the bottom of the block.
    edit_post_ wordpress_latest _wordpress
  5. Switch back to desktop mode and verify that the scrim fully covers the block
    edit_post_ wordpress_latest _wordpress
  6. Verify that an unselected block looks correct in both desktop and mobile mode

Types of changes

The CSS changes are tightly coupled to a selected block on a small screen so shouldn't affect anything else.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@johngodley johngodley added the Blocks label Aug 30, 2018

Fix invalid block scrim overflowing toolbar on mobile
On small screens the invalid block scrim overflows a block’s dropdown menu / toolbar, which is positioned beneath the block.
@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Aug 30, 2018

Contributor

@jasmussen thanks for the help with this. I implemented your pseudo CSS but found the bottom offset wasn't enough and had to change it to

$block-toolbar-height - $block-padding - 1px

I don't know if there's a better constant, or why it needs the -1px.

I also tied the change to .has-warning.is-selected as the block is shorter when unselected.

Contributor

johngodley commented Aug 30, 2018

@jasmussen thanks for the help with this. I implemented your pseudo CSS but found the bottom offset wasn't enough and had to change it to

$block-toolbar-height - $block-padding - 1px

I don't know if there's a better constant, or why it needs the -1px.

I also tied the change to .has-warning.is-selected as the block is shorter when unselected.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 31, 2018

Contributor

Solid PR, very nice work! That one pixel is the border width, so I pushed a tiny thing to fix to just use the variable, this will help explain why it's needed.

Contributor

jasmussen commented Aug 31, 2018

Solid PR, very nice work! That one pixel is the border width, so I pushed a tiny thing to fix to just use the variable, this will help explain why it's needed.

@jasmussen jasmussen self-requested a review Aug 31, 2018

@jasmussen jasmussen added this to the 3.8 milestone Aug 31, 2018

@jasmussen

👍 👍

jasmussen added some commits Aug 31, 2018

Tweak block warning quite a bit.
This commit changes the layout and design of the block warning a bit.

It is no longer obscuring content, and the scrim is lighter.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 31, 2018

Contributor

Okay, so I commandeered this PR, inspired by your fixes. And I pushed a great deal of changes in 3a6e1f3, enough that someone else should review this. The block warning looks like this now:

current

The big change here:

  • The block warning no longer obscures content, it makes the block taller to accommodate the warning.
  • The scrim is lighter so it's easier to see what the block is.
  • The hover style is not obscured by the warning
Contributor

jasmussen commented Aug 31, 2018

Okay, so I commandeered this PR, inspired by your fixes. And I pushed a great deal of changes in 3a6e1f3, enough that someone else should review this. The block warning looks like this now:

current

The big change here:

  • The block warning no longer obscures content, it makes the block taller to accommodate the warning.
  • The scrim is lighter so it's easier to see what the block is.
  • The hover style is not obscured by the warning

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Aug 31, 2018

@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Aug 31, 2018

Contributor

I like the block growing to show all of the content.

Looks good in all warning situations, except in a single-use block warning where the border disappears in places:

edit_post_ wordpress_latest _wordpress

However I think it's part of a larger issue to do with the block warning style and the single use error. I can push it out into another issue unless you think it's a quick fix here.

Contributor

johngodley commented Aug 31, 2018

I like the block growing to show all of the content.

Looks good in all warning situations, except in a single-use block warning where the border disappears in places:

edit_post_ wordpress_latest _wordpress

However I think it's part of a larger issue to do with the block warning style and the single use error. I can push it out into another issue unless you think it's a quick fix here.

@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Sep 1, 2018

Contributor

Giving it a go I got this in warning/style.scss

.editor-block-list__block:not(.has-warning) .editor-warning {
	border: $border-width solid $light-gray-500;
}

.editor-block-list__block.is-selected:not(.has-warning) .editor-warning {
	border-top: $border-width solid $light-gray-500;
}

But it has a double border when the block is selected. Not really sure of the best way of proceeding so I'll leave it to someone who knows what they are doing.

Contributor

johngodley commented Sep 1, 2018

Giving it a go I got this in warning/style.scss

.editor-block-list__block:not(.has-warning) .editor-warning {
	border: $border-width solid $light-gray-500;
}

.editor-block-list__block.is-selected:not(.has-warning) .editor-warning {
	border-top: $border-width solid $light-gray-500;
}

But it has a double border when the block is selected. Not really sure of the best way of proceeding so I'll leave it to someone who knows what they are doing.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 3, 2018

Contributor

Good catch.

I restored the full border. It's worth getting this warning in, then we can look at separate improvements, well, separately.

screen shot 2018-09-03 at 09 47 40

Contributor

jasmussen commented Sep 3, 2018

Good catch.

I restored the full border. It's worth getting this warning in, then we can look at separate improvements, well, separately.

screen shot 2018-09-03 at 09 47 40

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 3, 2018

Contributor

I like these changes. One thing that is jumping to me is that the option "Keep as HTML" could probably be "Keep as is" (better written, of course, maybe even just "Keep"). Being able to see the whole block clearly shows the content is not necessarily broken, just that it cannot be handled as a perfect block of a given type.

Contributor

mtias commented Sep 3, 2018

I like these changes. One thing that is jumping to me is that the option "Keep as HTML" could probably be "Keep as is" (better written, of course, maybe even just "Keep"). Being able to see the whole block clearly shows the content is not necessarily broken, just that it cannot be handled as a perfect block of a given type.

@talldan

talldan approved these changes Sep 5, 2018 edited

I like this - it's a nice improvement. I added one minor optional comment.

On the topic of copy, @mtias, I find "This block has been modified externally." to be a bit of a technical sentence that could be softened and it doesn't attempt to educate why there's an issue.

And 'Convert to Block' doesn't always make sense since it's already a block, just one with an warning state.

I also noticed that 'Convert to block' appears in both the main body of the error and the more menu and I couldn't see why.

Shall I make an issue to cover those things?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 5, 2018

Contributor

@talldan sure, can you add to #7604? That gives a general view of where we are heading.

Contributor

mtias commented Sep 5, 2018

@talldan sure, can you add to #7604? That gives a general view of where we are heading.

@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Sep 5, 2018

Contributor

Regarding the copy in the button and message I can look at those in another PR - it probably needs a more coordinated think.

I also noticed that 'Convert to block' appears in both the main body of the error and the more menu and I couldn't see why

Yep, I wasn't entirely sure whether this should be duplicated and just went with what was in the original mockup (although it has a different button). It's easy to remove so if it seems unnecessary I can do that elsewhere too.

Contributor

johngodley commented Sep 5, 2018

Regarding the copy in the button and message I can look at those in another PR - it probably needs a more coordinated think.

I also noticed that 'Convert to block' appears in both the main body of the error and the more menu and I couldn't see why

Yep, I wasn't entirely sure whether this should be duplicated and just went with what was in the original mockup (although it has a different button). It's easy to remove so if it seems unnecessary I can do that elsewhere too.

@youknowriad youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

@johngodley johngodley referenced this pull request Sep 6, 2018

Merged

Update invalid block copy #9667

0 of 4 tasks complete
@johngodley

This comment has been minimized.

Show comment
Hide comment
@johngodley

johngodley Sep 6, 2018

Contributor

Regarding the copy in the button and message I can look at those in another PR

#9667

Contributor

johngodley commented Sep 6, 2018

Regarding the copy in the button and message I can look at those in another PR

#9667

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 7, 2018

Contributor

@johngodley @jasmussen - I think this is good to merge, unless you can think of anything outstanding?

Contributor

talldan commented Sep 7, 2018

@johngodley @jasmussen - I think this is good to merge, unless you can think of anything outstanding?

@johngodley johngodley merged commit 851aec0 into master Sep 13, 2018

2 checks passed

codecov/project 50.23% (-0.01%) compared to c5573b7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johngodley johngodley deleted the fix/invalid-scrim-overflow branch Sep 13, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

🎉

Contributor

jasmussen commented Sep 13, 2018

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment