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

Restyle block warnings #5383

Merged
merged 5 commits into from
Mar 9, 2018
Merged

Restyle block warnings #5383

merged 5 commits into from
Mar 9, 2018

Conversation

nshki
Copy link
Contributor

@nshki nshki commented Mar 4, 2018

Description

This PR is to address #5201, where large blocks that throw a warning make the warning hard to find and interact with. The change shortens blocks with warnings with a max-height, and warnings hug the top of the blocks.

How Has This Been Tested?

  1. Created a paragraph block
  2. Wrote in a large amount of text (such that scrolling is necessary to see the full block)
  3. Edited the block as HTML
  4. Wrote in invalid HTML, e.g. <alskdjfl
  5. Blurred the block

Screenshots (jpeg or gifs if applicable):

screenshot-2018-3-4 hello world gutenberg wordpress

Checklist:

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

@bordoni
Copy link
Member

bordoni commented Mar 4, 2018

If the block has less than 500px, should we apply this?

@nshki
Copy link
Contributor Author

nshki commented Mar 4, 2018

@bordoni I think it's still applicable for blocks that don't exceed 500px -- it'll keep the warning position consistent across different blocks. Otherwise, short blocks would have centered warnings and taller ones wouldn't.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Nice work here 👍I'd prefer if the gitignore changes were not included here though.

.gitignore Outdated Show resolved Hide resolved
editor/components/warning/style.scss Outdated Show resolved Hide resolved
@nshki
Copy link
Contributor Author

nshki commented Mar 7, 2018

@aduth I went ahead and removed swap files from the .gitignore and rewrote the translate as translateX as requested!

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Code-wise this looks great, nice work 👍

I'm going to flag for design review because I'm curious if, after applying the max-height, it's necessary/ideal to still move the warning to the top of the block, or if the original issue is resolved by the application of a max-height alone.

cc @karmatosed @jasmussen

@aduth aduth added the Needs Design Feedback Needs general design feedback. label Mar 7, 2018
@jasmussen
Copy link
Contributor

I think it's fine to apply a max-height. I'm more curious what happens if the block is shorter than the warning itself. If the warning just expands the block to fit, then I think it's fine 👍 👍

@nshki
Copy link
Contributor Author

nshki commented Mar 8, 2018

@jasmussen Here's what happens when the block is shorter than the warning itself:

screenshot-2018-3-8 hello world gutenberg wordpress

The block expands to the min-height set at 250px. Should the warning stick to the top of the block (regardless of height) as shown in the screenshots or would it be better if they're centered?

@jasmussen
Copy link
Contributor

Interesting. This min-height, where's that from? Is that from the block or something that the error wrapper adds?

If it's from the block then I think it's fine. If it's from the wrapper then there shouldn't be a min-height.

@aduth
Copy link
Member

aduth commented Mar 8, 2018

It's applied to the block when a warning is present to avoid the case where the block itself is too short to display the warning (e.g. single line paragraph). But it was optimized for (a) when the prompt had more lines of text in it and (b) was vertically centered.

@jasmussen
Copy link
Contributor

I think we can probably reduce that min-height to 150 for now, merge this in, and then we can always look at additional improvements in the future. Perfect should not be the enemy of improvements here and now.

@nshki
Copy link
Contributor Author

nshki commented Mar 8, 2018

@jasmussen I think the 250px min-height was to also account for smaller viewports. With a 150px min-height, warnings gets cut off like the screenshot below:

screen shot 2018-03-08 at 09 47 49

@jasmussen
Copy link
Contributor

Oh indeed, then keepit as is and we'll look at improvements separately.

@nshki
Copy link
Contributor Author

nshki commented Mar 8, 2018

cc @aduth

@aduth
Copy link
Member

aduth commented Mar 9, 2018

Two ideas for (separate) enhancements:

  • Breakpoint-specific min-height
  • Assign the warning as the static element of the block, and the preview as absolute behind it, allowing height to adjust automatically to the warning plus some margin

@aduth aduth merged commit 6332d40 into WordPress:master Mar 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants