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 the button outline style for the old button markup #21816

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

youknowriad
Copy link
Contributor

closes #21747

The outline styles for the old button markup were not right. This PR fixes the styling to avoid the extra borders.

Testing instructions

  • Create buttons with the outline style with an old version (WP 5.4)
  • enable the plugin and this PR and check that buttons show up right on multiple themes.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Block] Buttons Affects the Buttons Block labels Apr 23, 2020
@youknowriad youknowriad self-assigned this Apr 23, 2020
@github-actions
Copy link

Size Change: +3 B (0%)

Total Size: 845 kB

Filename Size Change
build/block-library/style-rtl.css 7.19 kB +1 B
build/block-library/style.css 7.19 kB +2 B (0%)
ℹ️ 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.24 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 105 kB 0 B
build/block-editor/style-rtl.css 10.1 kB 0 B
build/block-editor/style.css 10.1 kB 0 B
build/block-library/editor-rtl.css 7.13 kB 0 B
build/block-library/editor.css 7.13 kB 0 B
build/block-library/index.js 112 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 57.7 kB 0 B
build/components/index.js 198 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.25 kB 0 B
build/data/index.js 8.43 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 569 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 28.2 kB 0 B
build/edit-post/style-rtl.css 12.5 kB 0 B
build/edit-post/style.css 12.5 kB 0 B
build/edit-site/index.js 10.8 kB 0 B
build/edit-site/style-rtl.css 5.25 kB 0 B
build/edit-site/style.css 5.24 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.3 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.32 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 711 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.49 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.67 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.17 kB 0 B

compressed-size-action

@youknowriad youknowriad requested review from jasmussen and kjellr April 24, 2020 13:28
@jasmussen
Copy link
Contributor

I'm afk for a bit, but evaluating the code, this looks correct to me! 👍👍

@andrewserong
Copy link
Contributor

andrewserong commented Apr 24, 2020

Thanks for putting together this fix @youknowriad! This change is testing well for me across the themes I mentioned in #21747 (Hestia, Lorina, 2019, 2020). I don't have approval access, but I'd otherwise give this PR a ✅ (whoops, turns out I do!)

Unrelated to this PR, I noticed that the new button markup in the 2019 theme doesn't appear to be overriding the Button styles correctly, but I imagine that'll need to be fixed in that theme separately.

image

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

So, I don't actually see any difference between this and master. This:

Screenshot 2020-04-24 at 15 56 29

Master:

Screenshot 2020-04-24 at 15 57 03

But maybe that's good? And with Andrews testing above, I'm approving!

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Nice clean fix, this change is testing well for me across the themes I mentioned in #21747 (Hestia, Lorina, 2019, 2020) 👍

Left a note regarding Button styling in 2019 theme, but that's unrelated to this PR.

@youknowriad
Copy link
Contributor Author

@jasmussen this fixes buttons with the old markup on the frontend, did you test with these?

@youknowriad youknowriad merged commit 7917af9 into master Apr 24, 2020
@youknowriad youknowriad deleted the fix/button-outline-style branch April 24, 2020 14:13
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 24, 2020
@jasmussen
Copy link
Contributor

I did not, but yes it's in the vein of your other fix so I can see that it should! Thanks.

@@ -48,7 +48,7 @@ $blocks-button__height: 56px;
border-radius: 0 !important;
}

.wp-block-button.is-style-outline,
Copy link
Member

Choose a reason for hiding this comment

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

👋 Riad, in this forum post a user reported that in WordPress 5.5, buttons with outline style and a preset color background didn't have the background color set.

My first instinct is to think that was how it was expected to work. However:

  • In 5.4 it worked differently, the backgrounds were also applied to buttons with outline style (both editor and front).
  • In 5.5, the editor and front reflect different values at the moment. If this is working as expected in the front, we should make it the same in the editor.

The button markup changed a bit during this cycle, so it's difficult to point to a specific PR that introduced the regression. This one is as good a candidate as any, as this PR increased the selector specificity to 030, while in WordPress 5.4 it was 020 (.is-style-outline .wp-block-button__link).

I've also tested in other themes and the results are somewhat inconsistent:

  • 2019: it works as before (uses background even with outline style) because the theme selector overrides the core selector by virtue of being loaded after (they have the same specificity).
  • 2017: it doesn't show the background when the outline style is selected (theme selector has lower specificity than core's).

I'm inclined to think this is a regression we need to fix. What are your thoughts on this @youknowriad ? cc @kjellr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for digging here. It's definitely not an intended change so I agree with considering this a regression.

Copy link
Member

Choose a reason for hiding this comment

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

Got a potential fix at #24599

@ellatrix do we have an incoming 5.5.1? This issue may be a good one to backport. If the fix is approved, should I backport it to the wp/5.5? Do I need to do anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I agree that it sounds like a regression. Thanks for flagging that!

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 [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button blocks using outline style in old markup render incorrectly
5 participants