Prevent accidental miss-clicks outside multi-select checkboxes #1700
Conversation
acelaya
force-pushed
the
multi-select-checkbox-fix
branch
from
13eef09
to
883b318
Compare
src/components/input/Select.tsx
Outdated
| '-m-2.5 p-2.5 border border-transparent', | ||
| // The checkbox is sized based on the container's font size. Make | ||
| // it a bit larger. | ||
| 'text-xl', |
There was a problem hiding this comment.
The checkbox was previously enlarged via scale-125, but that conflicts now with the margin and padding added here.
Increasing the font size was always the right approach.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1700 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 66 66
Lines 1115 1121 +6
Branches 435 439 +4
=========================================
+ Hits 1115 1121 +6 ☔ View full report in Codecov by Sentry. |
acelaya
force-pushed
the
multi-select-checkbox-fix
branch
2 times, most recently
from
4edaf9c
to
6818b76
Compare
|
I just tested this in the dashboard and realized this solution does not work if the option has multiple lines, so I'm putting it back in draft while I think in something else. |
acelaya
force-pushed
the
multi-select-checkbox-fix
branch
from
6818b76
to
9cc445c
Compare
I addressed this by using position absolute and zero top and bottom. That way it will always adapt to any height. EDIT: But that has other side effects I didn't consider: Grabacion.de.pantalla.desde.2024-09-06.14-45-06.webm |
acelaya
force-pushed
the
multi-select-checkbox-fix
branch
from
9cc445c
to
10346bc
Compare
| <div className="flex items-center"> | ||
| {optionChildren(children, { selected, disabled })} | ||
| </div> | ||
| {!multiple && ( | ||
| <CheckIcon | ||
| className={classnames('text-grey-6 scale-125', { | ||
| // Make the icon visible/invisible, instead of conditionally | ||
| // rendering it, to ensure consistent spacing among selected and | ||
| // non-selected options | ||
| 'opacity-0': !selected, | ||
| })} | ||
| /> | ||
| )} | ||
| {multiple && ( | ||
| <div | ||
| className={classnames('scale-125', { | ||
| 'text-grey-6': selected, | ||
| 'text-grey-3 hover:text-grey-6': !selected, | ||
| })} | ||
| > | ||
| <Checkbox | ||
| checked={selected} | ||
| checkedIcon={CheckboxCheckedFilledIcon} | ||
| elementRef={checkboxRef} | ||
| onChange={toggleValue} | ||
| onKeyDown={e => { | ||
| if (e.key === 'ArrowLeft') { | ||
| e.preventDefault(); | ||
| optionRef.current?.focus(); | ||
| } | ||
| }} | ||
| <div className="flex items-center"> |
There was a problem hiding this comment.
Since the wrapper is now items-stretch instead of items-center, every element that is now rendered inside of it needs to define its own flex container with items-center, so that everything is visually centered, while the checkbox wrapper (the label) is actually 100% tall, without having to use absolute containers, which have other side effects.
acelaya
force-pushed
the
multi-select-checkbox-fix
branch
from
10346bc
to
93abdbe
Compare
I moved back to a non-absolute-position approach, which has less side effects. Now the option container is |
|
I'm putting this back in draft, as in retrospect, the work I did on this and different attempts I mentioned above, made me realize there might be an easier way to achieve the same. Since now every "group" inside the option has its own flex container, we could remove the padding from the parent entirely, and then set individual paddings to every group where appropriate. I'll give that a try and then decide if this is still relevant. |
|
Closeing in favor of #1704 |
We have experienced (and more people has reported) that it is easy to miss-click outside a checkbox on a
MultiSelectoption. This is problematic, because clicking on a checkbox appends/removes the item to the selection, but clicking outside of it is captured as a click on the main option's body, causing the whole selection to be replaced with just that option.multi-select-current-2024-09-06_11.00.31.mp4
This PR changes that behavior, so that clicking on "the surroundings" of the checkbox behaves as clicking the checkbox itself.
"The surroundings" in this case means anywhere inside the right side of the option, matching the option's padding. That way the checkbox is always clicked if you click anywhere on the "right" of an option.
This shows how it behaves with these changes:
multi-select-fixed-2024-09-06_11.13.12.mp4
Note
This PR is easier to review ignoring whitespaces
Todo
scale-125.