Button Component: Update styling to avoid relying on Core Styles making it reusable #6562

youknowriad · 2018-05-03T13:58:59Z

Right now, the button component is relying heavily on core styles. This has some issues:

The need to add .core-ui to any place where we want to extend the default button styles
The components module is not reusable. One of the goals of the Gutenberg modules is to make them reusable in WP Admin and outside WP Admin as well. If we ever want to publish the components module on npm, it needs to define its own styles.
In components heavy application, we don't reuse classNames, we reuse components.

This PR copies the styles from Core into the components-button className to solve this.

Notes

I probably need to update the default styles according to the WP theme cc @jasmussen (I might need some help here)

Testing instructions

Navigate in the Gutenberg UI and check that the styling of the different buttons didn't regress.

youknowriad · 2018-05-03T13:59:31Z

components/button/index.js

 			'is-toggled': isToggled,
 			'is-busy': isBusy,
+			'is-link': isLink,


There are way too many button states here, but this matches Core, we can try to rethink some of them later.

Would it make sense to replace all these booleans with a more enum-like API? e.g. <Button style="link">?

I don't know maybe later, let's not change all the buttons today :)

afercia · 2018-05-03T15:53:55Z

There's one thing I've noticed also previously and this PR improves it but still a bit weird:

when I try to make an IconButton have also some text, the position of the icon is OK if the button is a real button, but it's not OK when it's actually a link. Seems it happens in Chrome, Firefox is OK. Seems the text-indent: 4px; doesn't work for links in Chrome. See the Preview button I've made it an IconButton for testing purposes:

Chrome:

Firefox:

noisysocks

Agree 100% with the motivation behind this change. It seems to cause a few visual regressions, however.

Various icons are misaligned:

There's a thick black border in a button's pressed/active state:

jasmussen · 2018-05-04T07:05:27Z

Thanks for doing this. I'll see if I can get time today to push a few visual fixes directly to this branch to address Roberts notes.

jasmussen · 2018-05-04T07:31:45Z

Pushed a fix to the alignment issue. Will look at some other issues with focus styles and admin theme colors in a minute.

@afercia

when I try to make an IconButton have also some text, the position of the icon is OK if the button is a real button, but it's not OK when it's actually a link. Seems it happens in Chrome, Firefox is OK. Seems the text-indent: 4px; doesn't work for links in Chrome. See the Preview button I've made it an IconButton for testing purposes:

Is this specific to this PR or was it also an issue before? The text-indent is definitely there to indent the text when there's an icon, and combined with flex it seems to have worked decently well. However if it doesn't work well in all situations or browsers, then I can take a look (as I added this initially). Feel free to create a new issue and assign me, and I'll look at it if it's not specific to this branch.

jasmussen · 2018-05-04T07:55:28Z

@youknowriad The theme colors regressed in this, because the CSS classes changed and additional specificity is required. That means the red color that the midnight theme applies to buttons with button-primary no longer works.

I can fix that in our _admin-schemes.scss mixin, but this is going to get messy for a variety of reasons, so before I do that, is there any other way we can approach this, so the core WordPress styles can still theme this?

Otherwise we're overriding buttons, and overriding theme styles. We're already doing this in a few places, sure, but if this is the path forward, we might as well detach these buttons as much as possible from the core UI, and see how many !important's we can remove in the process.

One other challenge with overriding the theme styles is additional colors. In the admin schemes mixin I've tried to limit things to two colors, the primary spot color and the secondary spot color. I've then used SCSS darken to create the additional shades necessary for the candy-colored "saving" indicator.

For the primary button I'm going to need all sorts of shades of the spot-color, for the border, for the pressed state, for the shadow, even the text shadow. I can try and re-create these using darken, but that won't be 100% accurate. The alternative is to duplicate every is-primary style for every admin scheme there is — but then we're duplicating yet again.

Is there any way we can keep using the core "button-primary" styles, instead of re-creating the is-primary style?

youknowriad · 2018-05-04T08:04:49Z

Thanks for the fixes @jasmussen :)

we might as well detach these buttons as much as possible from the core UI, and see how many !important's we can remove in the process.

I'm fine with that, that's exactly what this PR is doing, just leaving the same styles for now.

I can fix that in our _admin-schemes.scss mixin,

I think we can make try to automatically generated this mixin in the future. Like browsing our stylesheets and automatically add additional styles when we detect the usage of theme colors (similar to the RTL plugin). I think with good webpack skills (cc @aduth 😄 ), we can achieve that and don't think about these admin styles anymore. The good thing about such approach is that the theme styles would be loaded by stylesheet and not globally in a single stylesheet.

Is there any way we can keep using the core "button-primary" styles, instead of re-creating the is-primary style?

That defeats the purpose of this PR. We'll style rely on Core style in that case which needs specificity and break reusability.

jasmussen · 2018-05-04T08:17:52Z

I'll keep thinking about this and take a look next week if I have bandwidth.

youknowriad · 2018-05-15T12:38:58Z

Now that the post-css plugin is merged, I rebased this PR and updated the primary buttons styles to use theme colors. They don't match the current buttons perfectly but they're close enough. What do you think @jasmussen

jasmussen · 2018-05-16T10:06:11Z

I pushed some fixes, Riad. Can you take a look and see how it feels? Otherwise, feels like we're VERY close!

youknowriad · 2018-05-16T10:41:32Z

Looks good to me :) Let's get some eyes and keep this for the next release.

jasmussen · 2018-05-16T10:47:31Z

CC: @karmatosed let me know if you'd like some GIFs as the buttons change a little.

jasmussen · 2018-05-16T10:48:04Z

Also, the focus styles changed, let's make sure these are kosher.

karmatosed

Looks great, thanks.

…ng it reusable

@nfmohit

This fixes #6668, inspired by @nfmohit work in #6714 (props).

Feedback addressed

aduth · 2018-06-04T17:08:58Z

components/button/style.scss

+
+	/* Buttons that look like links, for a cross of good semantics with the visual */
+	&.is-link {
+		margin: 0;


Regression: This selector is more specific than (well, equal specific but out of order from) and thus overrides the margin applied from .components-color-palette_clear:

gutenberg/components/color-palette/style.scss

Line 9 in 94889af

margin-right: 20px;

v4.9.2 #6562

Honestly, we shouldn't need margin here for the button. The color palette panel shouldn't be doing a negative margin. It should have a container and spread the options via display: flex & align-items: space-between

youknowriad added the [Feature] UI Components Impacts or related to the UI component system label May 3, 2018

youknowriad self-assigned this May 3, 2018

youknowriad requested a review from a team May 3, 2018 13:58

youknowriad commented May 3, 2018

View reviewed changes

noisysocks previously requested changes May 4, 2018

View reviewed changes

youknowriad mentioned this pull request May 14, 2018

Build: using a postcss plugin to generate the admin-schemes styles #6739

Merged

jasmussen mentioned this pull request May 15, 2018

Buttons: wrong text color on focus #6668

Closed

youknowriad force-pushed the update/button-component branch 2 times, most recently from 89d5d36 to 712c093 Compare May 15, 2018 12:38

jasmussen mentioned this pull request May 16, 2018

Fix: Text colour on focused primary buttons #6714

Closed

4 tasks

jasmussen added the Needs Design Feedback Needs general design feedback. label May 16, 2018

jasmussen added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label May 17, 2018

karmatosed self-requested a review May 17, 2018 22:32

karmatosed approved these changes May 17, 2018

View reviewed changes

youknowriad and others added 3 commits May 21, 2018 12:46

Button Component: Update styling to avoid relying on Core Styles maki…

6d577a3

…ng it reusable

Fix admin schemes by using theme colors

31527d6

Fix issues with hover, active, focus states.

2465dc4

This fixes #6668, inspired by @nfmohit work in #6714 (props).

youknowriad force-pushed the update/button-component branch from e587426 to 2465dc4 Compare May 21, 2018 11:49

youknowriad merged commit ce61b59 into master May 21, 2018

youknowriad deleted the update/button-component branch May 21, 2018 15:15

mtias added this to the 3.0 milestone Jun 4, 2018

aduth reviewed Jun 4, 2018

View reviewed changes

talldan mentioned this pull request Sep 27, 2019

Tidy up button vertical align styles #17601

Merged

5 tasks

jasmussen mentioned this pull request Apr 14, 2021

Fix is-link style to take colors from wp-admin theme. #30823

Merged

7 tasks

ciampo mentioned this pull request Jun 5, 2023

Button: Add opt-in prop for larger isSmall size #51012

Merged

afercia mentioned this pull request Oct 11, 2024

Button: uniform text wrapping across variants #66049

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Button Component: Update styling to avoid relying on Core Styles making it reusable #6562

Button Component: Update styling to avoid relying on Core Styles making it reusable #6562

youknowriad commented May 3, 2018

youknowriad May 3, 2018

noisysocks May 4, 2018

youknowriad May 4, 2018

afercia commented May 3, 2018

noisysocks left a comment

jasmussen commented May 4, 2018

jasmussen commented May 4, 2018

jasmussen commented May 4, 2018

youknowriad commented May 4, 2018 •

edited

Loading

jasmussen commented May 4, 2018

youknowriad commented May 15, 2018

jasmussen commented May 16, 2018

youknowriad commented May 16, 2018

jasmussen commented May 16, 2018

jasmussen commented May 16, 2018

karmatosed left a comment

aduth Jun 4, 2018 •

edited

Loading

aduth Jun 4, 2018

Button Component: Update styling to avoid relying on Core Styles making it reusable #6562

Button Component: Update styling to avoid relying on Core Styles making it reusable #6562

Conversation

youknowriad commented May 3, 2018

youknowriad May 3, 2018

Choose a reason for hiding this comment

noisysocks May 4, 2018

Choose a reason for hiding this comment

youknowriad May 4, 2018

Choose a reason for hiding this comment

afercia commented May 3, 2018

noisysocks left a comment

Choose a reason for hiding this comment

jasmussen commented May 4, 2018

jasmussen commented May 4, 2018

jasmussen commented May 4, 2018

youknowriad commented May 4, 2018 • edited Loading

jasmussen commented May 4, 2018

youknowriad commented May 15, 2018

jasmussen commented May 16, 2018

youknowriad commented May 16, 2018

jasmussen commented May 16, 2018

jasmussen commented May 16, 2018

karmatosed left a comment

Choose a reason for hiding this comment

aduth Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

aduth Jun 4, 2018

Choose a reason for hiding this comment

youknowriad commented May 4, 2018 •

edited

Loading

aduth Jun 4, 2018 •

edited

Loading