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: setting a preset color on pullquote default style makes the block invalid #18194

Conversation

@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Oct 30, 2019

Fixes: #16429

The color mechanism on pull quotes is complex.
On the default style, the main color sets a border while on the solid color style, it sets a background.
The color mechanism uses a class for preset colors and does not set the color values on the attribute, but for borders, we don't have classes.
Previously we used the color mechanism normally for both styles and on the save function given that for presets colors, we don't have the color value as an attribute we queried the block-editor store settings to retrieve the color value and used the value in a border style.
The implementation is not correct, and it was a wrong decision because the save function becomes impure. Although not correct, It was working without block validation issues in the current WordPress version.
Meanwhile, other changes made this existing problem noticeable, and each time we set a preset color on the pullquote default style, the block becomes invalid.

This PR addresses the problem by on the default style even for preset colors setting the customMainColor attribute, instead of using the default color setting mechanism. This allows the color value to be directly available on the save function, so the function can be pure and depends on its attributes only.

How has this been tested?

I added a pull-quote block; I wrote some text, and I choose a preset color.
I added another pull-quote block; I wrote some text; I changed the block style to the solid color; I chose a preset color and verified it the editor applied it as a background; I changed the block style back to the default.
I saved the post with the two blocks and reloaded the editor and verified the blocks continue to be valid (on master they are not).

@jorgefilipecosta jorgefilipecosta force-pushed the fix/setting-a-preset-color-on-pullquote-default-style-makes-the-block-invalid branch from c642917 to 65af668 Oct 31, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 1, 2019

Hi @jorgefilipecosta, thanks for dealing with these issues. It seems to be pretty reasonable to make a new iteration to this implementation but wondering since we are going to make a refactoring whether it worths to take a look at the useColors() hook in order to avoid duplicating efforts #16781

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 1, 2019

Hi @jorgefilipecosta, thanks for dealing with these issues. It seems to be pretty reasonable to make a new iteration to this implementation but wondering since we are going to make a refactoring whether it worths to take a look at the useColors() hook in order to avoid duplicating efforts #16781

Hi @retrofox, this PR will be cherry-picked into WordPress 5.3, so we should try to keep it as simple as possible. The useColors hook is experimental and not part of WordPress 5.3, so I think we can not use it.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/setting-a-preset-color-on-pullquote-default-style-makes-the-block-invalid branch from 65af668 to 6775d92 Nov 1, 2019
@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 1, 2019

I haven't taken a look at the code yet but did to its functionality. Questions:

front-end env
Why does it apply the background color by inline styles when it's defined through a color palette? It seems that we are going to move to prefer the use of CSS classes instead of inline styles when it's possible.

#17832 (comment)

Screen Shot 2019-11-01 at 12 00 04 PM

Although the colors are being applied to the wp-block-pullquote element, there is a stronger rule that overrides what the PullQuote block applies, at least with my testing theme.

Screen Shot 2019-11-01 at 12 13 59 PM

Maybe we could add border-color: inherit to the <blockquote /> element ?
Screen Shot 2019-11-01 at 12 17 04 PM

@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 1, 2019

Hi @retrofox,

Why does it apply the background color by inline styles when it's defined through a color palette? It seems that we are going to move to prefer the use of CSS classes instead of inline styles when it's possible.

In this PR, we don't apply background colors by inline styles when they are defined through a color palette. We apply border colors with inline styles even if they are defined through a palette, and that was already the case. We do it because themes don't provide classes with border styles. For backgrounds, we use classes because themes provide background classes.

Although the colors are being applied to the wp-block-pullquote element, there is a stronger rule that overrides what the PullQuote block applies, at least with my testing theme.
Maybe we could add border-color: inherit to the

element ?

It seems its a collision with themes I guess we can try to improve the styles. But doing that, we may cause problems in other themes that currently work well. And this problem already happened on previous WordPress releases. Given that we are already late on the RC stage, I would prefer to try in a follow-up PR, not part of the release, and on this PR focus on the immediate issue.

@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 1, 2019

we don't apply background colors by inline styles

Got confused, I wanted to say to border-color. 🤦‍♂

Copy link
Contributor

retrofox left a comment

It seems its a collision with themes I guess we can try to improve the styles. But doing that, we may cause problems in other themes that currently work well. And this problem already happened on previous WordPress releases. Given that we are already late on the RC stage, I would prefer to try in a follow-up PR, not part of the release, and on this PR focus on the immediate issue.

Sorry, what I'm trying to say is that, if the user sets explicitly the border-color of the pull-quote, should not we try to apply these styles in the front-end without making collisions?

I've set the border color and the text color in the quote:

image

I expect to see these changes in the front-end. We're facing a similar case in the <NavigationMenu />, where the user is able to set up text and background-color of the menu.
The styles are being applied through either with CSS classes as well as inline styles, but also applying other rules if it's needed, conditionally to the has-text-color or/and has-background-color class/es.

Anyway, I've tested the PR and it works as (almost ;-)) expected. It's up to you pay attention to these comments. Maybe it worths to know the opinion of other folks. And finally, we can keep improving it later.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 2, 2019

How do currently invalid blocks work with this change? I mean what will happen when they open the editor, will the pull quote be updated, will the color be discarded?

@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 4, 2019

Just in case, there is a bug in prod right now when applying colors in the Pullquote:

  1. Create a PR block
  2. Set colors
  3. Hard refresh
    image
@jorgefilipecosta jorgefilipecosta force-pushed the fix/setting-a-preset-color-on-pullquote-default-style-makes-the-block-invalid branch from ceff8d2 to 42a1088 Nov 4, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 4, 2019

Hi @retrofox, @youknowriad, thank you for your questions/insights.

How do currently invalid blocks work with this change? I mean what will happen when they open the editor, will the pull quote be updated, will the color be discarded?

Hi @youknowriad, I found a solution to keep the named colors and migrate them to custom colors, so for the user, these changes should be transparent. The block should continue to be exactly as before.

Just in case, there is a bug in prod right now when applying colors in the Pullquote:

Hi @retrofox, this pull request is supposed to solve that bug. Are you able to replicate the problem in this branch, or you verified this PR solved the bug?

Sorry, what I'm trying to say is that, if the user sets explicitly the border-color of the pull-quote, should not we try to apply these styles in the front-end without making collisions?

The problem is that what we make to avoid collisions in one theme can have collisions in another theme. I think we can try to improve the styles to minimize common collisions in existing themes. But, I think it is not something we should try in this stage of the release.

@retrofox

This comment has been minimized.

Copy link
Contributor

retrofox commented Nov 4, 2019

But, I think it is not something we should try in this stage of the release.

Fair enough.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/setting-a-preset-color-on-pullquote-default-style-makes-the-block-invalid branch from 42a1088 to 7f6eccf Nov 4, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

jorgefilipecosta commented Nov 4, 2019

Merging this PR as it is a priority and the more testing it gets, the better. If anyone has any other suggestions, I will happily open a follow-up PR.

@jorgefilipecosta jorgefilipecosta merged commit fd5edfc into master Nov 4, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/setting-a-preset-color-on-pullquote-default-style-makes-the-block-invalid branch Nov 4, 2019
daniloercoli added a commit that referenced this pull request Nov 5, 2019
…rnmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView

* 'master' of https://github.com/WordPress/gutenberg: (56 commits)
  Update: Default gradients. (#18214)
  Fix: setting a preset color on pullquote default style makes the block invalid (#18194)
  Fix default featured image size (#15844)
  Fix postmeta radio regression. (#18183)
  Components: Switch screen-reader-text to VisuallyHidden (#18165)
  [rnmobile] Release 1.16.0 to master (#18261)
  Template Loader: Add theme block template resolution. (#18247)
  Add a README file for storybook directory (#18245)
  Add editor-gradient-presets to get_theme_support (#17841)
  Add "Image Title Attribute" as an editable attribute on the image block (#11070)
  enables horizontal movers in social blocks (#18234)
  [RNMobile] Add mobile Spacer component (#17896)
  Add experimental `ResponsiveBlockControl` component (#16790)
  Fix mover for floats. (#18230)
  Rename Component to WPComponent in docstring (#18226)
  Colors Selector: replace `Aa` text by SVG icon (#18222)
  Removed gif from README (#18200)
  makes the submenu items stacked vertically (#18221)
  Add block navigator to sidebar panel for nav block (#18202)
  Fix: consecutive updates may trigger a blocks reset (#18219)
  ...
jorgefilipecosta added a commit that referenced this pull request Nov 5, 2019
…k invalid (#18194)

* Fix: setting a preset color on pullquote default style makes the block invalid.

* Updated deprecation to keep the colors
jorgefilipecosta added a commit that referenced this pull request Nov 5, 2019
…k invalid (#18194)

* Fix: setting a preset color on pullquote default style makes the block invalid.

* Updated deprecation to keep the colors
pento pushed a commit to WordPress/wordpress-develop that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.

git-svn-id: https://develop.svn.wordpress.org/trunk@46656 602fd350-edb4-49c9-b593-d223f7449a82
nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.

git-svn-id: https://develop.svn.wordpress.org/trunk@46656 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.
Built from https://develop.svn.wordpress.org/trunk@46656


git-svn-id: http://core.svn.wordpress.org/trunk@46456 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Nov 5, 2019
The list of included fixes is:

WordPress/gutenberg#18183
WordPress/gutenberg#18194
WordPress/gutenberg#18230
WordPress/gutenberg#18275
WordPress/gutenberg#18287

Updated packages:
 - @wordpress/block-editor@3.2.4
 - @wordpress/block-library@2.9.5
 - @wordpress/edit-post@3.8.5
 - @wordpress/editor@9.7.5
 - @wordpress/format-library@1.9.4

Props @aduth, @mcsf, @youknowriad, @johnbillion.
Fixes #48502.
Built from https://develop.svn.wordpress.org/trunk@46656


git-svn-id: https://core.svn.wordpress.org/trunk@46456 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
…k invalid (WordPress#18194)

* Fix: setting a preset color on pullquote default style makes the block invalid.

* Updated deprecation to keep the colors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.