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

don't use hardcoded values for separator styles #17076

Closed
wants to merge 1 commit into from
Closed

don't use hardcoded values for separator styles #17076

wants to merge 1 commit into from

Conversation

aristath
Copy link
Member

Description

Avoid using hardcoded styles for separators.
This is a part of #16782, I'll try to push separate PRs per block so they can be examined and discussed separately.

Types of changes

  • Changed hardcoded font-size value from px to em
  • Changed $dark-gray-100 color to currentColor with an opacity of .4 which is pretty close on white backgrounds.

The above changes make things work properly regardless of font-size used on the site, and regardless of whether the site uses a light or dark background color.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Block] Separator Affects the Separator Block [Type] Enhancement A suggestion for improvement. labels Aug 19, 2019
@draganescu draganescu added the Needs Design Feedback Needs general design feedback. label Nov 22, 2019
@draganescu
Copy link
Contributor

Tested this small update and it works all right but it does change the look of the separator to be lighter (in TwentyTwenty at least). Adding NeedsDesignFeedback.

Before:

Screenshot 2019-11-22 at 17 10 14

After:

Screenshot 2019-11-22 at 17 09 46

margin-left: auto;
margin-right: auto;
opacity: .4;
Copy link
Member

Choose a reason for hiding this comment

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

@richtabor
Copy link
Member

We discussed this in today's design meeting (Slack). While we could remove the hard coded value for opacity as well, it's not super necessary.

If we do remove the opacity value, I personally think an opacity control is not necessary. I'm for removing the hard opacity (or leaving it as is without an additional control).

@paaljoachim paaljoachim removed the Needs Design Feedback Needs general design feedback. label May 26, 2020
@ZebulanStanphill
Copy link
Member

If we do remove the opacity value, I personally think an opacity control is not necessary. I'm for removing the hard opacity (or leaving it as is without an additional control).

What opacity control are you talking about? I think you (or I) may have misunderstood something somewhere?

@richtabor
Copy link
Member

If we do remove the opacity value, I personally think an opacity control is not necessary. I'm for removing the hard opacity (or leaving it as is without an additional control).

What opacity control are you talking about? I think you (or I) may have misunderstood something somewhere?

In the meeting we discussed possibly considering an opacity control (if we are to remove all hardcoded opacity value as well).

@ZebulanStanphill
Copy link
Member

Ah, okay. Well, adding an opacity control seems over-the-top to me. I would think that the theme should be in charge of most of the separator styling. (Actually, I'm not sure why we even have multiple style variations included in core.)

@richtabor
Copy link
Member

Ah, okay. Well, adding an opacity control seems over-the-top to me. I would think that the theme should be in charge of most of the separator styling. (Actually, I'm not sure why we even have multiple style variations included in core.)

So do you think we should remove the current opacity: .4; CSS value?

@ZebulanStanphill
Copy link
Member

I'm ambivalent about it, since it's in a theme.scss file, which is for opt-in opinionated styles, if I recall correctly. It having an opacity value set is irrelevant to whether or not there should be an opacity control.

@richtabor
Copy link
Member

Im fine either way, although I thought the goal was to remove overly specific styles?

@ZebulanStanphill
Copy link
Member

ZebulanStanphill commented Jun 10, 2020

I think the goal of this PR was to make the block styles (both default and opt-in) work better with themes, by using values that adapt better (e.g. em units instead of px units and currentColor + opacity instead of always a dark gray that may not match the text color of some themes).

@aristath
Copy link
Member Author

aristath commented Aug 4, 2020

Yeah, the default styles for separators were to use a color $dark-gray-100.
Assumng a white background and black text, that corresponds to a currentColor and an opacity of 0.4.
The opacity is not really necessary and can definitely be removed, I was simply trying to keep the styles as close to the original ones as possible (hence I added an opacity).
As long as the color changes to currentColor instead of the dark-gray the rest is not that important. Without this change, themes that respect the user's color-scheme preferences (or even just dark themes) need to override the default styles and write their own.

@aristath aristath mentioned this pull request Aug 4, 2020
6 tasks
@ZebulanStanphill
Copy link
Member

Yeah, the opacity shouldn't really be a blocker for this PR. Just add a leading zero to the number, rebase the PR to get tests working, and let's ship it. A follow-up PR can remove the opacity if desired.

@ZebulanStanphill ZebulanStanphill added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. CSS Styling Related to editor and front end styles, CSS-specific issues. labels Aug 4, 2020
@aristath
Copy link
Member Author

aristath commented Aug 5, 2020

Unfortunately I can't push code to this PR any longer... The source fork no longer exists and it can't be changed in the PR's reference.
I created a new one instead on #24366, and pushed the requested changes there.

I'll go ahead and close this PR in favor of the newer one since we can push any requested updates there. 👍

@aristath aristath closed this Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Separator Affects the Separator Block CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants