Skip to content
This repository was archived by the owner on Dec 2, 2020. It is now read-only.

Fix up separator styles #423

Merged
merged 6 commits into from
Oct 14, 2020
Merged

Fix up separator styles #423

merged 6 commits into from
Oct 14, 2020

Conversation

kjellr
Copy link
Collaborator

@kjellr kjellr commented Oct 12, 2020

  • Fixes the width of the default separator.
  • Adds support for wide & full alignments. (Coming in GB 9.2)
  • Fixes the opacity of the separator in the editor. (Noticed while testing with GB trunk 🤷‍♂️)

The last two bullets aren't possible/issues with the plugin turned off currently, but that first one definitely is. Screenshots below are with GB trunk active.


Front End

Before After
Screen Shot 2020-10-12 at 10 42 24 AM Screen Shot 2020-10-12 at 10 41 36 AM

Editor

Before After
Screen Shot 2020-10-12 at 10 42 38 AM Screen Shot 2020-10-12 at 10 41 25 AM

@kjellr kjellr added the [Component] Default blocks Issues related to adjusting default blocks label Oct 12, 2020
@kjellr kjellr self-assigned this Oct 12, 2020
@melchoyce
Copy link
Contributor

FYI, getting an error when I try to build this.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 12, 2020

Good catch. The IE build styles couldn't handle having an externally-referenced var in our global variables file. 😕

In 6bb57f3, I removed the --separator--width variable and replaced it with our usual --responsive--aligndefault-width variable instead. This should be fine, since it's the same anyway.

@melchoyce
Copy link
Contributor

Not seeing anything to do with opacity, but tested this on 5.5.1 with and without Gutenberg nightly and it works well 👍

One concern — it feels weird to change the default style to wide or full and not have the separator change in appearance. I understand the logic there, since it's "default width," but I wonder if it might be worth allowing that to get wider as well, since it's an intentional setting someone is selecting. (It feels less weird with dots since they're a narrow width to begin with)

@carolinan
Copy link
Contributor

carolinan commented Oct 13, 2020

I agree, I found it confusing that the wide style was thicker, not wider.

Maybe I misunderstood the intention but with Gutenberg trunk, the default style does not work with the new alignment option.

Suggestions:
Add a new block style for the thick version.

If there are core styles that are not meant to used because they don't work with the new setting, unregistered that style.

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 13, 2020

One concern — it feels weird to change the default style to wide or full and not have the separator change in appearance. I understand the logic there, since it's "default width," but I wonder if it might be worth allowing that to get wider as well, since it's an intentional setting someone is selecting. (It feels less weird with dots since they're a narrow width to begin with)

That's actually just a bug that I didn't notice. 😄 Note that in the front end it's working as expected. I'll fix in the editor later today.

Copy link
Contributor

@melchoyce melchoyce left a comment

Choose a reason for hiding this comment

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

Good design-wise as soon as that width issue is fixed 👍

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@kjellr
Copy link
Collaborator Author

kjellr commented Oct 14, 2020

Good design-wise as soon as that width issue is fixed

Fixed.

Add a new block style for the thick version.
If there are core styles that are not meant to used because they don't work with the new setting, unregistered that style.

I'll open a separate PR to try this out.

@kjellr kjellr dismissed melchoyce’s stale review October 14, 2020 12:22

Issue is fixed. 👍

@kjellr kjellr merged commit 0cd9278 into trunk Oct 14, 2020
@kjellr kjellr deleted the fix/separator-styles branch October 14, 2020 12:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
[Component] Default blocks Issues related to adjusting default blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants