-
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
Try: Reduce checkbox size in data views #60475
Conversation
Size Change: +1.57 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
Much prefer this visually |
I did some research regarding this suggestion.
|
No strong opinions here, it seems fine to contextually reduce the size.
It should probably be considered in this calculus that commonly a checkbox will be paired with a label, making the whole label in addition to the checkbox a clickable area. In this particular context, the entire row is clickable, making a mammoth size tap target. |
Cool, I suppose the next step is to refactor this PR to add a size variant to CheckboxControl. I'll work on that when I get a moment. Heads up @WordPress/gutenberg-components. |
Not sure if we can or not, but would it make sense to not add the variant quite yet, and keep it override-only in this context, at least for a couple of releases to gather some feedback? Mainly to be sure when we finally do add the variant, we actually intend to use it in more places than just here. |
We can do it that way if y'all prefer. Might need to add some more overrides to finesse the icons though, they're a bit cramped at the minute. |
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. |
I think there are three directions:
There is #58564 regarding alignment changes for this component. It seems that local overrides are considered best to avoid there. Furthermore, if I look deeper into the history of this component, the checkbox size was originally 16px. And it seems that it was changed to 20px in #20464. The reason why a 20px size checkbox looks unnatural is because the area itself is small, such as a cell in a table layout or a card in a grid view, right? I'd love to hear feedback from @WordPress/gutenberg-components on what direction we should go. |
Good point. Also the overrides in this PR don't fully take into account the style changes that occur in narrow viewports, so that kind of complication might be another reason not to address this in a local override. I'll add another potential approach to @t-hamano's list, which is to try using a CSS variable in the CheckboxControl styles, e.g. |
Smaller check looks good, in practice it's also worked well in Gmail that uses a similar size, even if the hit area is way larger than the visual footprint (as it also is here). Entirely separately, that 16px "Search" font size is really awkward. I'm aware it exists to avoid zooming on iOS, is that still necessary or are there other ways around that these days? |
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.
I like the smaller size there, but we didn't add the click-to-select behavior to the cards in grid layout yet. If y'all feel that'd be a pre-requisite I'd be happy to restrict the smaller size to table layout initially. |
white-space: nowrap; | ||
border: 0; | ||
.dataviews-view-table-selection-checkbox { | ||
line-height: 0; |
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.
Without this the checkbox wrapper renders at 18.58px tall which throws off the horizontal alignment with the smaller checkboxes.
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.
I pushed a change to simplify the size styles a bit, and added a changelog for the component. I think this is good to go from the wp-components standpoint — let me know if I'm missing something!
$checkbox-input-size: 20px; | ||
$checkbox-input-size-sm: 24px; // Width & height for small viewports. | ||
.components-checkbox-control { | ||
--checkbox-input-size: 24px; // Width & height for small viewports. |
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.
Let's keep the size for small viewports private for now, since we don't need to override 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.
Scratch that. I discussed this with @DaniGuardiola and decided that we shouldn't be exposing an official-looking CSS var for this sizing experiment. Made some changes on how we override it. On the whole though, this style refactor should allow more of a safer override, although not "officially" sanctioned.
|
||
@include break-small() { | ||
left: -2px; | ||
top: -2px; | ||
--checkmark-size: calc(var(--checkbox-input-size) + 4px); |
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.
Currently in trunk, the icon size is 24px regardless of whether the input is 20px or 24px. I left it that way, but we could simplify the code (and possibly correct the visual incongruence) if that was unintended.
label { | ||
position: absolute; |
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.
Sorry for the pain here, we do eventually want to make this kind of checkbox layout possible without hacks! I'll bump the priority.
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.
@jameskoster I was thinking about this in the context of #60799 and realized it was already possible. Quick fix proposed in #60835.
@jasmussen We could at least maintain the placeholder size, if that's better? trunk...placeholder-size |
@mirka thanks for working on this :) It seems the latest changes result in a strange appearance on small screens: |
@jameskoster Good catch! It was due to some styles coming in from the |
@jameskoster Also, I noticed that the checkboxes are hidden until hovered on all devices/viewports. Shouldn't they be shown by default on touch devices? |
Flaky tests detected in 3aeb44f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8692109353
|
Probably yes, but I think that's a detail to fix separately since the issue already exists on trunk. I'll open a PR this week. If you're happy with the state of this PR I think it's good to merge :) |
@jameskoster Sounds good, let's merge 👍 |
Code is copied from WordPress/gutenberg#60475.
Given the close proximity and large number of checkboxes in data views, the default 20px size feels a little clumsy and adds outsized prominence to bulk actions.
This PR is not intended to be merged, it's purely to try a smaller checkbox in data views to see if it works better. If so, then it'd be good to update the Checkbox component to support aAs pointed out by Joen, it may actually be preferable to start with these style overrides, and add the variant later.small
size variant (with finessed iconography) and utilise that within data views.The smaller footprint could arguably be considered a usability trade-off, but - given the entire row is clickable to select in table layout - perhaps this isn't such a compromise?
Trunk
This branch