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

Revert the button block to the previous markup #21923

Merged
merged 3 commits into from
Apr 27, 2020
Merged

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Apr 27, 2020

The button markup change in #21266 created some backward compatibility issues for theme authors. Noting that we were aware of these breakage (styling for new buttons on the frontend), but sometimes hard decisions are required. This markup change was specially important for Global styles. But since the Global styles project is not shipped as stable yet, this PR reverts the markup change to give us more time before this potential breaking change. I'd still encourage theme authors to update their styles to only rely on wp-block-button__link without requiring the presence of a wp-button parent or not which would make the styles work regardless of the chosen markup in the end.

closes #21917
closes #21909

@github-actions
Copy link

github-actions bot commented Apr 27, 2020

Size Change: +1.25 kB (0%)

Total Size: 818 kB

Filename Size Change
build/block-library/editor-rtl.css 7.03 kB -21 B (0%)
build/block-library/editor.css 7.03 kB -20 B (0%)
build/block-library/index.js 114 kB +1.29 kB (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.23 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 106 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.14 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.42 kB 0 B
build/date/index.js 5.47 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.1 kB 0 B
build/edit-navigation/index.js 3.54 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/index.js 27.8 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 11 kB 0 B
build/edit-site/style-rtl.css 5.26 kB 0 B
build/edit-site/style.css 5.25 kB 0 B
build/edit-widgets/index.js 8.33 kB 0 B
build/edit-widgets/style-rtl.css 5 kB 0 B
build/edit-widgets/style.css 5 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.4 kB 0 B
build/editor/style-rtl.css 3.27 kB 0 B
build/editor/style.css 3.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.63 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 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 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 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 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.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@youknowriad youknowriad marked this pull request as ready for review April 27, 2020 15:56
@youknowriad youknowriad added the [Block] Buttons Affects the Buttons Block label Apr 27, 2020
@aduth aduth added this to the Gutenberg 8.0 milestone Apr 27, 2020
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

It seems on this PR when I apply a predefined color with 2020 the color is not viewable on the editor. The same also happened on master but it does not happen on WordPress 5.4. I wonder if the colors not being applied was a regression from #21266 that we are keeping?

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I verified that the markup of the blocks is the same that we had on 5.4 and I did not found any regression when comparing against master.

@aduth
Copy link
Member

aduth commented Apr 27, 2020

It seems on this PR when I apply a predefined color with 2020 the color is not viewable on the editor. The same also happened on master but it does not happen on WordPress 5.4.

While I'm unable to reproduce it on master, I do see the same issue. Interestingly, I think it's a side-effect of the fact that this "fixes" theme styling (#21917, #21909). You can see in the screenshot below that the color from theme styles (from TwentyNineteen) has a higher specificity than the predefined color styling:

image

Like you, I did find that it worked as expected on the front-end. In fact, the changes here help restore theme styles which were not working correctly after #21266, in addition to using the correct predefined color assigned to the button.

As far as interoperability with the specific theme styles I'm seeing, I think this is in a better state than master, at least so far as the colors working correctly on the front-end. It would be good if we can get the colors interoperating better with the theme styles in the editor. For the purposes of today's RC release and if Riad has finished for the day, for me it would make sense to merge this with the above as an acknowledged "Known Issue", ideally with a resolution before Wednesday's final release.

@aduth
Copy link
Member

aduth commented Apr 27, 2020

While I'm unable to reproduce it on master

It appears this may be theme-dependent. On TwentyTwenty, I see the same problems with predefined colors not taking effect in the editor, both on this branch and on master. On TwentyNineteen, predefined colors work correctly on master but not in this branch. It seems the difference is how the selectors differ between TwentyNineteen and TwentyTwenty. As seen in the screenshot of my previous comment, the TwentyNineteen themes try to style this by assuming separate elements .wp-block-button and .wp-block-button__link, hence the remark about the fact that this is expected as a result of "fixing" the markup to respect theme styles.

@youknowriad
Copy link
Contributor Author

@jorgefilipecosta @aduth are you testing with the mu-plugins that disable colors enabled or not? this could have an impact here.

@aduth
Copy link
Member

aduth commented Apr 28, 2020

@youknowriad I am. But even disabling the behavior of it, the problem persists.

buttons-colors

@BinaryMoon
Copy link

I'd still encourage theme authors to update their styles to only rely on wp-block-button__link without requiring the presence of a wp-button parent or not which would make the styles work regardless of the chosen markup in the end.

This is the best thing to do but it doesn't guarantee the styles will work.

In the current structure the modifiers (.is-style-outline) get applied to the wrapper, and not the button itself, so you still have to do some CSS gymnastics to make it work consistently for new and old structures.

@strarsis
Copy link
Contributor

Hehe, I just finished refactoring the styles to match the new-old button markup. 😄

@youknowriad
Copy link
Contributor Author

@strarsis that's a good thing because in theory they work on both markups right? and it's a win if it ever becomes new again 😬

@unprintedch
Copy link

Will it stay lke that?
I like CSS gymnastic but just to know....

In the current structure the modifiers (.is-style-outline) get applied to the wrapper, and not the button itself, so you still have to do some CSS gymnastics to make it work consistently for new and old structures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block
Projects
None yet
6 participants