-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Align checkbox, radio, and toggle input design #63490
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +70 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
Dimensions and metrics look great. As we've talked about in chat, I'm personally unsure about the font weight change, but I'll let you reach a broader group when you're ready for that, and I'll defer to group consensus. But it would be useful to see these change in context, not just storybook, if you can include a couple of screenshots of the editor with these components and their weight shown there. |
Good point, I added some 'changes in context' screenshots. The font-weight is a tricky one. Objectively judging the components in isolation I feel it's a good change because it viaully differentiates the label from regular body text, which seems like an a11y enhancements. However in some circumstances I see how it can feel a bit overweight. But that might suggest there are other styles in the type scale to address, rather than the component styles being 'wrong'. Keen to gather more feedback around that. |
How much do we take into consideration non-Latin scripts when making legibility decisions? This is what the proposed RadioControl would look like with Chinese/Japanese characters, for example:
Aside from the heading label which is already hard to read even on a high-res screen with quality fonts, I feel like this adds even more legibility issues by adding extra weight to small text. Also note that all caps styling will not add any visible contrast in languages that do not have a lower/upper case distinction. (@t-hamano Does the legibility issue ever come up in your circles?) In my experience it is hard to achieve a single style that works in all languages, and it is possible to set slightly different styles based on the |
I have never heard anyone around me suggest that the readability of the text is an issue in the UI of the entire editor, including the Another issue here is that, as this CSS variable indicates, the font depends on the OS and the fonts installed on your machine, so readability may vary depending on your environment. For example, on my Windows machine, English text looks like this. Personally, I feel the
Strangely,
|
Just my two cents: I find the smaller controls rather a regression than an improvement. |
Thanks for the feedback y'all!
That would suggest an issue with the all-caps label style, no? I still think there's value in differentiating labels from body text, but acknowledge it's a tricky detail to get right and probably one to explore independently. I reset the weight change in this PR and updated the OP. |
Attempt to make it harder for someone to remove it accidentally
I went ahead and pushed a couple tweaks, if that's ok. They're mostly just architectural fixes, which shouldn't change the resulting visual. The only visual changes I added are the |
packages/base-styles/_variables.scss
Outdated
$toggle-width: $grid-unit-40; | ||
$toggle-height: $grid-unit-20; | ||
$toggle-border-width: 1px; | ||
$toggle-thumb-size: $grid-unit-15; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to lift all these variables up into base-styles
? I'm not sure how conservative we are about adding global variables, but personally I wouldn't add them if it isn't necessary to share them across packages.
If it's just to share values between the FormToggle and ToggleControl components, we can keep them within the @wordpress/components
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. On a separate note, at some point we should look at tidying up shared styles in the package: some variables are defined in Sass, some as CSS custom properties, and some in JS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's just to share values between the FormToggle and ToggleControl components
That was it. Your suggestion makes sense, though I'm not sure how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved back to the FormToggle styles in 0d73cc5. (Ideally this should use a @use
to scope the variables better, but we're not set up to do that properly right now.)
Absolutely! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good from my end, let's add a changelog before merging 👍
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com> Committed via a GitHub action: https://github.com/Automattic/jetpack/actions/runs/10684158857 Upstream-Ref: Automattic/jetpack@fe2a3d1
* Update wordpress monorepo * js-packages/components: ToggleControl: Update styles for WordPress/gutenberg#63490 That change started setting a height on the control itself (versus just the stuff inside it), so update our styles to override that too. * Add `__nextHasNoMarginBottom` all over the place The vast majority are inside a `PanelBody`, which already overrides margins. Had to fix one margin on the simple-payments block. --------- Co-authored-by: Renovate Bot <bot@renovateapp.com> Co-authored-by: Brad Jorsch <brad.jorsch@automattic.com>
This PR is to scrutinize some design system explorations about better aligning
ToggleControl
,RadioControl
, andCheckboxControl
.Changes
cursor: pointer
acrossTo test
npm run storybook:dev
RadioControl
,CheckboxControl
,ToggleControl
Changes in context