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

Update buttons and icons buttons size and consistency #19058

Merged
merged 11 commits into from Dec 12, 2019
Merged

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Dec 11, 2019

I've noticed that our current Button components have inconsistent sizes (between IconButton and regular Button), So I was working on consolidating the APIs and making sure everything is consistent.

While doing so we have to make some decisions about the size of the default buttons, small and large.
So I went with 36px for the default size (currently the default for the IconButton), 24px for the small size (like today).

I decided to remove the large size because it doesn't make sense when the default button size is 36px.

Once this PR lands, we should follow-up with a Core ticket for consistency across all WP-Admin.

I also added a story to show all the different variations in one single place.

Capture d’écran 2019-12-11 à 11 08 18 AM

Breaking changes

There's a few style breaking changes, I don't think these will have big impacts on third party usage though.

  • The main breaking change is that there's a defined height in the "Button" component. If you're using the "Button" component and expecting its height to grow with the content, you should override the height with height: auto.
  • Another change here is that small or large buttons don't receive the "default" style variations by default, it should be explicit.

Testing instructions

  • Check all the buttons in Gutenberg :)
&:not(:disabled):not([aria-disabled="true"]):not(.is-default):hover {
// Ideally, we should remove these styles, but they're kept
// Because historically, the icon buttons have different hover styles
// than the regular buttons.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 11, 2019

Author Contributor

Right now we expect icon buttons across Gutenberg UI to have the dark borders on hover while the default buttons (without any variations) don't have this UI. This forces us to have these overrides here.

Ideally, we don't need this and the IconButton should only have to deal with icons, everything else should be deferred to the regular Button component and style.

cc @jasmussen

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 11, 2019

Author Contributor

This is fixed in the last commit 5d35623 but it means that if use a Button without specifying a variation and we expect a custom hover styles, we'd have to override the default, !important is the simplest way to achieve that.

@youknowriad youknowriad requested a review from mapk Dec 11, 2019
@youknowriad youknowriad requested a review from Soean as a code owner Dec 11, 2019
@gziolo gziolo mentioned this pull request Dec 11, 2019
6 of 6 tasks complete
@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 11, 2019

Awesome work, you are my hero. I had a really hard time trying to unify some of the styles related to buttons 😅

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 11, 2019

I will let others comment to, but this all feels like a great opportunity to go larger not smaller. I think almost keeping the larger size over smaller could be ace. Bringing in with this the newer UI feels like a great step.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 11, 2019

Impressive work simplifying things here. This is the most visible change as the publish/preview buttons are now 2px taller:

Screenshot 2019-12-11 at 12 13 53

Paired with other spacing efforts, this is good, as it moves the default button closer to the grid system.

The icon buttons should remain square, though, regardless of size, with the vertical ellipsis currently being the only intentional exception to that rule:

Screenshot 2019-12-11 at 12 16 12

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

Since the IconButton should remain square (if no text used) and the icon size is 20px this means the padding should be 8px on each size and not 10 and ideally consistent on all buttons including icon buttons.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

I think almost keeping the larger size over smaller could be ace.

@karmatosed I'm removing the isLarge variation because the default is larger.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 11, 2019

I was about to report the issues with misplaced icons in the icon button but you were faster with 994654f. 🎉

I couldn't find any regressions in comparison with recent refactorings. Awesome work. There is still one issue with spacing in the toolbar for buttons, but it exists in master so it should be tackled separately:
Screen Shot 2019-12-11 at 12 24 08
Screen Shot 2019-12-11 at 12 24 12

I noticed also a slight change in the hover state for menu items. It brings consistency so I don't think it's concerning. However, it should be approved by designers.

this branch
Screen Shot 2019-12-11 at 12 06 15

master
Screen Shot 2019-12-11 at 12 06 54

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 11, 2019

Since the IconButton should remain square (if no text used) and the icon size is 20px this means the padding should be 8px on each size and not 10 and ideally consistent on all buttons including icon buttons.

I think that's perhaps a simplistic take. I agree in code purity principle, but due to how browsers render text, we often have to massage the paddings and make exceptions. There are some gnarly details in https://youtu.be/jnV1u67_yVg?t=642

This is what I see now:
Screenshot 2019-12-11 at 12 23 11

The padding left and right for normal buttons here does seem a little tight, honestly I think it could be much more relaxed, but I acknowledge your iconbutton point. It's probably worth moving forward with what we have here as it's not terrible, and we need to acknowledge that Mobile is a factor in making the buttons wider.

But perhaps in a followup PR, I would love to tweak the horizontal padding further for buttons to make it more harmonious. This horizontal padding additionally serves the purpose of ambiguating from an input field, which is especially important given the more minimalist style of buttons currently.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 11, 2019

I think those text buttons in the toolbar look a bit different but it still feels okay.

HTML focused, Preview hovered and pressed
Screen Shot 2019-12-11 at 12 09 32

HTML hovered, Preview pressed
Screen Shot 2019-12-11 at 12 10 15

<Button isPrimary isSmall>Primary Button</Button>
<Button isDefault isSmall>Secondary Button</Button>
<Button isTertiary isSmall>Tertiary Button</Button>
<IconButton isSmall icon="ellipsis" />

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 11, 2019

Member

Not that I care that much, but something to consider.
Shouldn't be IconButton documented under its own component?

For reference, this is how sizes are presented for the Icon component.

https://wordpress.github.io/gutenberg/?path=/story/components-icon--sizes

@ItsJonQ, it's something we should standardize and wrap with some helper component so we don't have to repeat those wrappers and styles over and over again. Ideally, we could code it along those lines:

<Variations label="Small Buttons">
	<Button isSmall>Button</Button>
	<Button isPrimary isSmall>Primary Button</Button>
	<Button isDefault isSmall>Secondary Button</Button>
	// ...
</Variations>

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 11, 2019

Author Contributor

to be honest, I think we can remove the IconButton component entirely in favor of an icon prop in the Button component (that's separate from this PR though).

the main reason I used a single story is to be able to visualize all variations in the same screen.

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 11, 2019

Member

to be honest, I think we can remove the IconButton component entirely in favor of an icon prop in the Button component (that's separate from this PR though).

There is an issue for that for sure. I like the idea :)
You should merge this PR as soon as it's confirmed by someone from the design team :)

This comment has been minimized.

Copy link
@ItsJonQ

ItsJonQ Dec 11, 2019

Contributor

@ItsJonQ, it's something we should standardize and wrap with some helper component so we don't have to repeat those wrappers and styles over and over again.

@gziolo I think that's a great idea :). I started doing that for the G2 Prototypes I've been making

https://tz3ir.csb.app/#/buttons

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 11, 2019

Member

You are always ahead of time @ItsJonQ, this is exactly what I had in mind :)

@gziolo
gziolo approved these changes Dec 11, 2019
@youknowriad youknowriad requested review from nerrad, noisysocks and ntwb as code owners Dec 11, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

Ok I went ahead and renamed isDefault to isSecondary. I wonder if this PR fixes the audit issue.

@youknowriad youknowriad force-pushed the update/buttons branch 2 times, most recently from 33d3359 to 5cfbe97 Dec 11, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 11, 2019

I restored the hover styles of the dropdown menus.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 11, 2019

I'm removing the isLarge variation because the default is larger.

Ah sorry, that got lost in the description for me. That's great :)

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Dec 11, 2019

One note:

Paired with other spacing efforts, this is good, as it moves the default button closer to the grid system.

@jasmussen what would get us to the grid system size now on buttons and could that come in with this? I am conscious we have a lot of little changes, which are great but moving together on toolbar and interaction points like buttons seems sensible.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 11, 2019

what would get us to the grid system size now on buttons and could that come in with this?

I'm thinking of an 8px grid system with options for 4 and 12px usage. With icons being 24px and a block toolbar being 48px, 36px is compatible (9x4). Right now there's 28px some places, and 34px in others, so it's a bit messy. 12/24/36/48 is technically base12 but is 8px grid compatible it feels like.

I don't think it's necessarily something that has to happen with this PR, other than the capacity in which it already is (small = 24px, large = 36px). I think Dave Whitley had created a grid scale that might be a good next step?

@youknowriad youknowriad force-pushed the update/buttons branch from cacd0cf to 45759a8 Dec 11, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Dec 12, 2019

thanks all for the feedback, it looks like we're all in favor of this change. I'm well aware that this might introduce small issues here and there but I'm choosing to merge it soon in order to find these as soon as possiible.

@youknowriad youknowriad merged commit bfc58f5 into master Dec 12, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/buttons branch Dec 12, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Dec 13, 2019

Sorry for not catching this, but this PR appears to have caused a small issue with the button colors.

Before:

before

The gray color is #555d66 (which is $dark-gray-500).

After:

after

The color here is unset and defaults to what the browser provides, which is a semi-opaque black that translates to #222222 on a white background.

The last HEAD that worked was the one before this one in the commitlist, b2ef0f550b5e0fc549742f3dfbef2bea89155169.

margin: 0;
border: none;
background: none;
color: $dark-gray-500;

This comment has been minimized.

Copy link
@jasmussen

jasmussen Dec 13, 2019

Contributor

Removing this is possibly what caused the regression. If the color shouldn't live here, then it needs to live elsewhere so the button can inherit it, for example using currentColor.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 13, 2019

Author Contributor

My main issue is that the color shouldn't be different between Button and IconButton so we should move this to the Button stylesheet.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 13, 2019

Author Contributor

Fix here #19123

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.