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

Fix: Cover block does not contain a class for predefined gradients #20921

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Mar 16, 2020

Fixes: #20825

The class has-background-gradient was not present for predefined gradients on the frontend (it was present on the editor as expected). This caused a visual difference between the editor and the frontend.
This PR fixes the issue and adds the required class to the frontend.

How has this been tested?

I added a cover image with a background image and a predefined gradient.
I verified the class has-background-gradient was part of the produced markup.
I created a cover block in the WordPress 5.4 RC with a background image and a predefined background. I pasted the markup produced and saved a post with it. I opened the post with Gutenberg on this branch and verified the block was not considered invalid and the class was automatically added to the markup.

…redefined gradients
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Mar 16, 2020

Size Change: +97 B (0%)

Total Size: 857 kB

Filename Size Change
build/block-library/index.js 111 kB +97 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.03 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 100 kB 0 B
build/block-editor/style-rtl.css 10.7 kB 0 B
build/block-editor/style.css 10.7 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 191 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.22 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.21 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.62 kB 0 B
build/edit-post/style.css 8.62 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.45 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 779 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Mar 16, 2020

In reviewing the discussion from #20825, it's not clear to me whether the class name is expected to be applied, or if it's a problem of the styles of has-background-gradient needing to be applied when those gradientClass is also applied. Or was it considered practically infeasible, or better that the cover block styles not be "aware" of the full set of possible gradient variation classnames?

@@ -64,7 +64,7 @@ export default function save( { attributes } ) {
{
'has-background-dim': dimRatio !== 0,
'has-parallax': hasParallax,
'has-background-gradient': customGradient,
'has-background-gradient': gradient || customGradient,
[ gradientClass ]: ! url && gradientClass,

This comment has been minimized.

Copy link
@aduth

aduth Mar 16, 2020

Member

There is at least one condition here ! url (not sure if gradientClass can be falsey) in determining whether a gradient class is applied. Should that also be taken into consideration above when considering to apply has-background-gradient?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 16, 2020

Author Member

Hi @aduth when we have a gradient background, and a URL background we need to make the gradient background part of another element (we use a span just for that) and we apply the class to the other element that's why we use "! url && gradientClass". The idea of "has-background-gradient" is to allow targeting cover blocks that use a gradient background no matter if they also use an image background or not. I guess it would be possible to target the gradients using their gradients classes but themes can defined their own so one would need to use something like class attribute contains but that sounds hacky, also one would need to target the application of the class to this element or the span used when we don't have an image background. This class seems to make things easier.
The changes to the save make the save match what happens in the editor where we add the class provided that a gradient exists (custom or predefined). gradientClass will always be defined provided gradient attribute exists so "gradient || customGradient" or using "gradientClass || customGradient" is exactly the same.

This comment has been minimized.

Copy link
@aduth

aduth Mar 16, 2020

Member

So in the serialized output:

<div class="wp-block-cover has-background-dim has-background-gradient" style="background-image:url(data:image/jpeg;base64,/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=)"><span aria-hidden="true" class="wp-block-cover__gradient-background has-midnight-gradient-background"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->

It's expected that the has-background-gradient class is on a different element (div) than the has-midnight-gradient-background element (span), when a url is assigned?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Mar 16, 2020

Author Member

Yes it is expected our selectors already rely on this e.g:"&.has-background-dim:not(.has-background-gradient)::before,".

@aduth
aduth approved these changes Mar 16, 2020
@jorgefilipecosta jorgefilipecosta merged commit 8905f96 into master Mar 16, 2020
4 checks passed
4 checks passed
build
Details
pull-request-automation
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
WordPress 5.4 Must Have automation moved this from Needs Review to Done Mar 16, 2020
@jorgefilipecosta jorgefilipecosta deleted the fix/cover-block-does-not-contain-grandient-class-when-a-predefined-gradient-is-selected branch Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.