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

Notices: Editor notice margins appear conflicted with theme background color #18762

Closed
aduth opened this issue Nov 26, 2019 · 9 comments · Fixed by #18871
Closed

Notices: Editor notice margins appear conflicted with theme background color #18762

aduth opened this issue Nov 26, 2019 · 9 comments · Fixed by #18871
Assignees
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Design Feedback Needs general design feedback. [Package] Editor /packages/editor [Status] In Progress Tracking issues with work in progress

Comments

@aduth
Copy link
Member

aduth commented Nov 26, 2019

Describe the bug

If a theme defines an editor background color, the existence of a notice in the editor will create an undesirable gap of white.

image

To reproduce

Steps to reproduce the behavior:

  1. Activate a theme which has an editor background color (e.g. TwentyTwenty)
  2. Navigate to Posts > Add New
  3. Open your browser's Developer Tools Console, then add the script:
    • wp.data.dispatch( 'core/notices' ).createInfoNotice( 'Hello World!' )

Expected behavior

Any space between notices and the editable content should be seamless (inherit background color).

Desktop (please complete the following information):

  • OS: macOS Catalina 10.15.1 (19B88)
  • Browser: Chrome Version 78.0.3904.108

Additional context

Originally observed at #18732 (comment)

My first intuition was to remove the margin from the last notice in the set. However, there are two issues with this:

  • The margin should be preserved between the last notice of the "pinned" collection, and the first notice of the "dismissible" collection
  • I would expect that even the intentional margins between notices should have the theme background color show through.

Possible solutions:

  • Remove the margins altogether (use borders instead to create seams between notices?)
  • Move notice rendering into .editor-styles-wrapper element (the element which receives the theme styling)
@aduth aduth added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Package] Editor /packages/editor Needs Design Feedback Needs general design feedback. labels Nov 26, 2019
@richtabor
Copy link
Member

Yup, it's from a margin-bottom of 5px added by the .components-notice styling indeed. With multiple notices involved, here's what we have now:

Screen Shot 2019-11-26 at 12 59 36 PM

If we move the notices into the .editor-styles-wrapper element as is, they'd overlap the .editor-post-title element, causing a conflict there. Perhaps a border, or inset box-shadow, could be a simpler implementation.

Screen Shot 2019-11-26 at 1 00 14 PM

Screen Shot 2019-11-26 at 1 04 07 PM

@richtabor
Copy link
Member

Here's with setting.components-notice margin to 0 and adding border-bottom: 1px solid rgba(0, 0, 0, 0.15) to the notice (including the first one, which makes the notice feel part of the UI a bit more). I chose that border-bottom color to match the .edit-post-header border with transparency, so that it does that clash with notice colors.

Screen Shot 2019-11-26 at 1 09 23 PM

And here's the same, but with two notices:

Screen Shot 2019-11-26 at 1 11 55 PM

Info notices:

Screen Shot 2019-11-26 at 1 12 21 PM

Error notices:

Screen Shot 2019-11-26 at 1 12 59 PM

@aduth
Copy link
Member Author

aduth commented Nov 26, 2019

@richtabor I'll of course defer to your judgment (and any other designers who might weigh in), but from my perspective, your proposed border seems nicer to me even than the original design was going for with the margins. There's a nice consistency with borders used elsewhere in the UI (with that in mind, should it be the exact color of other borders, if not already?).

Seems to me like an easy way to fix the issue 👍

@richtabor
Copy link
Member

...but from my perspective, your proposed border seems nicer to me even than the original design was going for with the margins.

100%, totally agree.

There's a nice consistency with borders used elsewhere in the UI (with that in mind, should it be the exact color of other borders, if not already?).

Yea, we can try - it's worth exploring. There may be a bit of clash with a gray border against the notices (as they hold a background color), which is why I went transparent. For reference, the 0 0 0 1px rgba(0, 0, 0, 0.2) I mocked here is also used within the ColorPaletteControl component, as those button elements have background colors as well.

Screen Shot 2019-11-26 at 1 39 46 PM

@aduth
Copy link
Member Author

aduth commented Dec 2, 2019

There may be a bit of clash with a gray border against the notices (as they hold a background color), which is why I went transparent.

Huh, I clearly needed to brush up on my understanding of how borders and backgrounds interact (CodePen), since I was under the impression it wouldn't matter if the border was outside of the content/background area. But yes, I can see now why a semi-transparent black would likely clash less than a static gray border.

@aduth
Copy link
Member Author

aduth commented Dec 2, 2019

For reference, the 0 0 0 1px rgba(0, 0, 0, 0.2) I mocked here [...]

Can you clarify, since your first comment mentioned using a border-bottom, but the syntax you mention here now (and in use by ColorPaletteControl) is a box-shadow; should we be using a border or box shadow? I would imagine a border should be sufficient?

@richtabor
Copy link
Member

For reference, the 0 0 0 1px rgba(0, 0, 0, 0.2) I mocked here [...]

Can you clarify, since your first comment mentioned using a border-bottom, but the syntax you mention here now (and in use by ColorPaletteControl) is a box-shadow; should we be using a border or box shadow? I would imagine a border should be sufficient?

Oh sorry about that! I was referring to the rgba value being the same. A border-bottom would work great here.

For clarification:

  1. The screenshot below is using a border-bottom the same value as the edit-post-header:

Screen Shot 2019-12-02 at 2 08 35 PM

  1. While this screenshot is using a border-bottom: 0 0 0 1px rgba(0, 0, 0, 0.125) (which adds more contrast between elements and fits in better with the contrast from the edit-post-header border-bottom. I'd say this works much better.

Screen Shot 2019-12-02 at 2 09 02 PM

@aduth
Copy link
Member Author

aduth commented Dec 2, 2019

Thanks for clarifying. In #18871, I went with border-bottom. I used 0.2 as the alpha though. That's different from what you're suggesting in your latest comment, but per your comment about "the rgba value being the same", the color palette and friends use 0.2 as well, not 0.125.

@richtabor
Copy link
Member

richtabor commented Dec 3, 2019

...the color palette and friends use 0.2 as well, not 0.125.

Yea, I was playing around with the rgba to see if I could get the contrast visually similar to what the .editor-post-header provides.

Screen Shot 2019-12-02 at 7 47 23 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Design Feedback Needs general design feedback. [Package] Editor /packages/editor [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants