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

Increase radio dimensions to match checkboxes #27377

Merged
merged 4 commits into from
Dec 9, 2020
Merged

Conversation

jameskoster
Copy link
Contributor

The dimensions of radio inputs is currently 16px while checkboxes are 20px. This PR updates radios to match checkboxes.

Before:

Screenshot 2020-11-30 at 13 03 06

After:

Screenshot 2020-11-30 at 13 35 57

Screenshot 2020-11-30 at 13 37 22

@jameskoster jameskoster added the General Interface Parts of the UI which don't fall neatly under other labels. label Nov 30, 2020
@jameskoster
Copy link
Contributor Author

I also updated how the central dot on the active state is styled, so that it no longer needs dedicated rules (inside a media query) to adapt to other dimensions.

@github-actions
Copy link

github-actions bot commented Nov 30, 2020

Size Change: +24.3 kB (2%)

Total Size: 1.21 MB

Filename Size Change
build/a11y/index.js 1.14 kB +1 B
build/annotations/index.js 3.8 kB +2 B (0%)
build/autop/index.js 2.83 kB -1 B
build/block-directory/index.js 8.72 kB -2 B (0%)
build/block-editor/index.js 128 kB -107 B (0%)
build/block-editor/style-rtl.css 11.2 kB +1 B
build/block-editor/style.css 11.2 kB +1 B
build/block-library/editor-rtl.css 9.07 kB +127 B (1%)
build/block-library/editor.css 9.07 kB +124 B (1%)
build/block-library/index.js 149 kB +1.23 kB (0%)
build/block-library/style-rtl.css 8.35 kB +84 B (1%)
build/block-library/style.css 8.35 kB +83 B (0%)
build/block-serialization-default-parser/index.js 1.88 kB +1 B
build/blocks/index.js 48.1 kB +2 B (0%)
build/components/index.js 172 kB +19 B (0%)
build/components/style-rtl.css 15.4 kB +52 B (0%)
build/components/style.css 15.3 kB +53 B (0%)
build/compose/index.js 10.2 kB +284 B (2%)
build/core-data/index.js 15.4 kB +518 B (3%)
build/data/index.js 8.97 kB -6 B (0%)
build/date/index.js 31.8 kB +20.6 kB (64%) 🆘
build/deprecated/index.js 769 B +1 B
build/edit-navigation/index.js 11.1 kB -1 B
build/edit-post/index.js 306 kB +436 B (0%)
build/edit-post/style-rtl.css 6.49 kB +71 B (1%)
build/edit-post/style.css 6.47 kB +67 B (1%)
build/edit-site/index.js 24.7 kB +632 B (2%)
build/edit-site/style-rtl.css 3.93 kB +25 B (0%)
build/edit-site/style.css 3.93 kB +23 B (0%)
build/edit-widgets/index.js 26.3 kB -10 B (0%)
build/editor/index.js 43.4 kB +135 B (0%)
build/editor/style-rtl.css 3.84 kB -16 B (0%)
build/editor/style.css 3.84 kB -11 B (0%)
build/element/index.js 4.62 kB -4 B (0%)
build/format-library/index.js 6.74 kB -126 B (1%)
build/html-entities/index.js 622 B -1 B
build/i18n/index.js 3.57 kB +3 B (0%)
build/list-reusable-blocks/index.js 3.1 kB -3 B (0%)
build/nux/index.js 3.42 kB -2 B (0%)
build/plugins/index.js 2.54 kB -19 B (0%)
build/priority-queue/index.js 789 B -2 B (0%)
build/redux-routine/index.js 2.84 kB -4 B (0%)
build/reusable-blocks/index.js 2.92 kB -3 B (0%)
build/rich-text/index.js 13.4 kB -4 B (0%)
build/server-side-render/index.js 2.77 kB -2 B (0%)
build/shortcode/index.js 1.69 kB +1 B
build/viewport/index.js 1.86 kB +1 B
build/wordcount/index.js 1.22 kB +1 B
ℹ️ View Unchanged
Filename Size Change
build/api-fetch/index.js 3.42 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-library/theme-rtl.css 789 B 0 B
build/block-library/theme.css 790 B 0 B
build/block-serialization-spec-parser/index.js 3.06 kB 0 B
build/data-controls/index.js 827 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.95 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-widgets/style-rtl.css 3.13 kB 0 B
build/edit-widgets/style.css 3.13 kB 0 B
build/editor/editor-styles-rtl.css 476 B 0 B
build/editor/editor-styles.css 478 B 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.27 kB 0 B
build/is-shallow-equal/index.js 698 B 0 B
build/keyboard-shortcuts/index.js 2.54 kB 0 B
build/keycodes/index.js 1.93 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.31 kB 0 B
build/notices/index.js 1.82 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/primitives/index.js 1.43 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 2.84 kB 0 B
build/warning/index.js 1.14 kB 0 B

compressed-size-action

@jasmussen
Copy link
Contributor

This is awesome and I'm excited to approve this — the code looks good too.

However for whatever reason I can't get the branch to compile — how are you testing this? I'm not seeing the proper size here:

Screenshot 2020-12-01 at 09 34 48

@jameskoster
Copy link
Contributor Author

Ah, I temporarily swapped the checkbox in the screenshot above to a radio to test as I couldn't remember where we actually used radios hah.

It looks like the updated size is not being applied in the visibility options because they are not actually consuming the radio control component. They are however using the radio-control mixin for style. I'll see if I can update that so that all radios are styled correctly, regardless of whether they're utilised in the component or not.

@jameskoster
Copy link
Contributor Author

Done. I took the liberty of fixing the password input while I was in there too.

Before:

Screenshot 2020-12-01 at 10 00 05

After:

Screenshot 2020-12-01 at 09 56 56

@jasmussen
Copy link
Contributor

Very nice, this is working well for me. I'm excited to land this:

Screenshot 2020-12-01 at 11 33 27

However I wonder whether the width: 50%; height: 50%; is a good approach. Specifically, this radio has special rules for Windows 10 high contrast mode — the white border exists for that purpose. But I wonder if that border shows up a as a dot or a circle now. I haven't had a chance to test in windows high contrast mode, and I know that's painful to do, but I think we might need to test that first.

@jameskoster jameskoster added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Dec 1, 2020
@jameskoster
Copy link
Contributor Author

the white border exists for that purpose

Just to make sure before I begin this expedition, which border are you referring to?

@jasmussen
Copy link
Contributor

Yes, sorry — the white dot isn't actualy just a div with a border radius and background color. It has a white border as well. The point is, this white border becomes a black border in windows 10 high contrast mode, whereas background colors need to be removed.

My plate is full today, but if it's a nightmare hellride for you to test in Windows 10, I'll set aside some time next week to test and push some fixes to your branch.

@jameskoster
Copy link
Contributor Author

The best I am able to wrangle is Edge with a high contrast extension installed. I'm not sure if that achieves the same result as the native High Contrast Mode though.

Either way, here's what I see on Master:

Screenshot 2020-12-03 at 13 24 16

And here's what I see on this branch:

Screenshot 2020-12-03 at 13 27 15

@jasmussen
Copy link
Contributor

Finally had a chance to look at this one, and indeed the problem was as I suspected. The border I referred to is being made opaque, whereas the background is removed. Causing this:

Capture

I tried making that border 4px, which lands us this:

tweaked

With a full 5px border the dot becomes solid, but then it's also bigger than its dimensions and gets shifted around.

Can we make the dot be exactly 8x8px? If yes, then the 4px border inside should work. Otherwise, I think just making it 4px is acceptable. Let me know what you think.

@jameskoster
Copy link
Contributor Author

The dot can indeed be 8x8 :) That latest commit should do the trick.

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.

Nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants