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

add color support to separator block #16784

Merged
merged 7 commits into from
Jul 31, 2019
Merged

Conversation

senadir
Copy link
Contributor

@senadir senadir commented Jul 27, 2019

tackles a point from #16483

it adds color support to the Separator block.

worth mentioning, the color won't be honored if the theme already defined a color, this is the case for the Dots, not the line, I can extend it to honor the theme color in the case of the line HR but it will require printing direct CSS or that the theme style is defined it using !important.

so, this is what is happening

  • line HR color is overriding the theme style (unless the theme uses !important)
  • the Dots are respecting the theme style (since they use currentColor)

you will see this case in 2019 in which they define a color for the dots

my suggestion is to unify the experience, but this will cause a slight problem.

the Dots are defined using :before, in order to unify the experience, we have to do this inside React rather then using :before but it will break compantility with anything that is already targeting the :before, like 2019

@senadir senadir added [Block] Separator Affects the Separator Block [Type] Enhancement A suggestion for improvement. labels Jul 27, 2019
packages/block-library/src/separator/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/separator/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/separator/save.js Outdated Show resolved Hide resolved
packages/block-library/src/separator/save.js Outdated Show resolved Hide resolved
packages/block-library/src/separator/style.scss Outdated Show resolved Hide resolved
@youknowriad
Copy link
Contributor

you will see this case in 2019 in which they define a color for the dots

@kjellr Do you think this should be fixed in 2019 or maybe force the styling using !important?

@kjellr
Copy link
Contributor

kjellr commented Jul 29, 2019

Do you think this should be fixed in 2019 or maybe force the styling using !important?

Twenty Nineteen is likely not the only theme doing this. Since Gutenberg was providing a color here, I'm sure other themes are overriding it too. Even so, I'd prefer not using !important — the "right" way to fix this would be in Twenty Nineteen (and any other themes that do this).

So yes, we should open a Trac ticket for Twenty Nineteen once this PR is merged in.

@youknowriad
Copy link
Contributor

Thanks @kjellr It seems that this PR requires a dev note.

@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Jul 29, 2019
@youknowriad
Copy link
Contributor

Code-wise this PR is in a good state, can we get some design thoughts.

@youknowriad youknowriad added the Needs Design Feedback Needs general design feedback. label Jul 29, 2019
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

Hey @senadir! Thanks for the PR.

I think this is missing an update to separator/theme.scss in order to get his working correctly on the front end. By default, that provides a light gray bottom border to themes that include theme support for wp-block-styles. I'm seeing that gray show up alongside the specified color on the front end for the Gutenberg Starter theme:

Editor:

Screen Shot 2019-07-29 at 7 49 35 AM

Front end:

Screen Shot 2019-07-29 at 7 47 37 AM

@mapk
Copy link
Contributor

mapk commented Jul 30, 2019

I just tested this and it's looking good. Thanks for creating the PR @senadir!

✅ The colors show in the style variations beautifully.

Screen Shot 2019-07-29 at 4 55 36 PM


I also experienced the oddity on the frontend that @kjellr pointed out. Here it is zoomed in.

Screen Shot 2019-07-29 at 4 57 10 PM

@senadir
Copy link
Contributor Author

senadir commented Jul 30, 2019

native behavior of HR is to provide color via border-color, since we don't have color classes for borders, I'm using color & background (Dots & line respectively).
the theme.scss is also defining a border so I'm deleting it.

frontend
image

editor
image

@kjellr
Copy link
Contributor

kjellr commented Jul 30, 2019

the theme.scss is also defining a border so I'm deleting it.

I don't think this works as intended — we do want to provide a default front-end color for themes that opt in to wp-block-styles. We only need to delete/override this border when a background is applied. Otherwise, it clears the default color entirely, meaning that the "Default" and "Wide Line" variations are invisible by default, (unless a theme provides styles for them).

Here's what I see testing these latest changes in the Gutenberg Starter Theme (which provides no editor styles, and just uses wp-block-styles to pull in separator styles on the front end):

master in front end and the editor:

Screen Shot 2019-07-30 at 9 50 31 AM

This branch, in front end and the editor:

Screen Shot 2019-07-30 at 9 45 13 AM

As you can see, the first two separators are invisible now. If I specify a color for each one in the editor, they'll show up again:

Screen Shot 2019-07-30 at 9 44 42 AM

Additionally, it looks like this PR shortens the height of the Default (short) separator style to 1px. I tend to prefer the previous 2px height, and I think we should stick to that unless we have solid reasoning for changing it.

Thank you!

@senadir
Copy link
Contributor Author

senadir commented Jul 30, 2019

this commit should fix this, changes are not scoped to the style
(Gutenberg starter theme/master frontend)
image

(2019 frontend)
image

(2019 editor style is still inconsistent between the frontend & the preview because it uses height for the frontend & border for the editor)
image

@kjellr Gutenberg starter theme behavior seems the most optimal, but I fear 2019 behavior is common
do you think we should force remove the border inside the editor for cases like 2019? or leave it for theme owners to fix it?

@senadir senadir requested a review from kjellr July 30, 2019 21:51
@kjellr
Copy link
Contributor

kjellr commented Jul 31, 2019

I left a quick inline note about one of the updates. Other than that, this is looking pretty good.

I'm really hesitant to resort to !important rules here, so I'm still thinking the best way to do this would be to fix that Twenty Nineteen problem in the theme itself. We'll also add a dev note when this is merged, so that theme authors know to check their themes.

I'm definitely happy to hear perspective from other theme authors though!

@senadir
Copy link
Contributor Author

senadir commented Jul 31, 2019

@kjellr do you think this is ready to merge now?

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

👍 This is good to go. I have a patch ready for Twenty Nineteen, and will link back here once I open the Trac ticket.

@kjellr
Copy link
Contributor

kjellr commented Jul 31, 2019

The trac ticket for Twenty Nineteen is available here: https://core.trac.wordpress.org/ticket/47811

@senadir senadir merged commit e4b1337 into WordPress:master Jul 31, 2019
@senadir senadir deleted the better-hr branch July 31, 2019 18:36
@github-actions github-actions bot added this to the Gutenberg 6.3 milestone Jul 31, 2019
@youknowriad youknowriad mentioned this pull request Aug 19, 2019
22 tasks
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* add color support to separator block

* add color to frontend render

* add support for dots style in frontend

* fix review problems, update block.json file

* fix border issue with theme.scss

* extend border remove in editor

* fix problems with no color selected
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* add color support to separator block

* add color to frontend render

* add support for dots style in frontend

* fix review problems, update block.json file

* fix border issue with theme.scss

* extend border remove in editor

* fix problems with no color selected
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Oct 29, 2019
Comment on lines +11 to +13
// Override any background themes often set on the hr tag for this style.
// also override the color set in the editor since it's intented for normal HR
background: none !important;
Copy link
Member

Choose a reason for hiding this comment

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

I see there was some discussion about the use of !important here in the pull request comments, and separately a fix addressing a TwentyTwenty-specific incompatibility at Trac#47811.

Which leaves the question: Do we actually need it? Can it be removed?

See also: #19539

Alternatively, I wonder:

  • Only including this specific style rule if .has-text-color and/or .has-background, representative of the fact that the default should only need to be reset when the user chooses to apply a custom color (correct me if I'm mistaken)
  • Can it be addressed by assuring that the specificity of this selector would always "defeat" a theme's styling? Especially considering we would probably want this reset only in the case of applying when a custom color is used, i.e. adding .has-text-color and/or .has-background as additional modifier classes:
.wp-block-separator.is-style-dots.has-background,
.wp-block-separator.is-style-dots.has-text-color {
	background: none;
}

Choose a reason for hiding this comment

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

What if a different method for generating dots was used?
The current method is a lot of CSS, that is a hassle for themes to override. I was going for the minimal style of making dots with multiple background images using radial gradients, but the current style using !important wouldn't let me.
I also couldn't use a dotted border because all those other styles I would have to override.

So what if the core style is the radial gradient instead of an important no background?

Copy link
Member

Choose a reason for hiding this comment

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

It'd be worth exploring a simpler approach.

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 Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants