fix: [sc-27302] Multiselect filter correctly disables chips in modal; #782
Conversation
apattersonATX-HB
requested review from
stephenh,
bdow,
KoltonG,
ksk5280,
chr1sjf0x and
DeanGilewicz
as code owners
|
This pull request has been linked to Shortcut Story #27302: [FE] Comp Filter Options based on comps returned. |
src/inputs/ToggleChipGroup.tsx
Outdated
| ...Css.white.bgLightBlue700.$, | ||
| ":hover:not(:isDisabled)": Css.bgLightBlue800.$, | ||
| } | ||
| : { ":hover:not(:isDisabled)": Css.bgGray300.$ }), |
There was a problem hiding this comment.
Question: is :isDisabled correct? I've seen :disabled:
https://developer.mozilla.org/en-US/docs/Web/CSS/:disabled
But not :isDisabled. Does this somehow magically work with the data-disabled attribute?
I was expecting per:
https://stackoverflow.com/a/17249001
Something like :not([data-disabled="true"])
There was a problem hiding this comment.
I inserted :isDisabled because react-aria's useCheckboxGroupItem props extends InputBase type down the chain which does have the isDisabled prop. It'll actually work without using data-disabled attribute so maybe it's just a quirk of react-aria?
But yea probably better to do it the traditional CSS way.
src/inputs/ToggleChipGroup.tsx
Outdated
| import { Label } from "src/components/Label"; | ||
| import { Css } from "src/Css"; | ||
| import { useTestIds } from "src/utils/useTestIds"; | ||
|
|
||
| type ToggleChipItemProps = { | ||
| label: string; | ||
| value: string; | ||
| isDisabled?: boolean; | ||
| disabledReason?: string; |
There was a problem hiding this comment.
Suggestion: Instead of two separate props, we typically model this as a union, like:
export interface BeamButtonProps {
/**
* Whether the interactive element is disabled.
*
* If a ReactNode, it's treated as a "disabled reason" that's shown in a tooltip.
*/
disabled?: boolean | ReactNode;
That way the user can't do something like isDisabled=false disabledReason="some error" and it's ambiguous that maybe we really be disabled, like does a set disabledReason override the isDisabled is false?
There was a problem hiding this comment.
Thats true. Had tunnel vision on the filter so hadn't considered ToggleChipGroup ever using anything besides a string
apattersonATX-HB
force-pushed
the
sc-27302/fix-modal-multifilter-disable-options
branch
from
0bfd31f
to
4b099a4
Compare
apattersonATX-HB
deleted the
sc-27302/fix-modal-multifilter-disable-options
branch
### [2.235.4](v2.235.3...v2.235.4) (2023-01-09) ### Bug Fixes * [sc-27302] Multiselect filter correctly disables chips in modal; ([#782](#782)) ([1acfc0c](1acfc0c))
|
🎉 This PR is included in version 2.235.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Multiselect filter will now correctly disable options when rendered as a ToggleChipGroup.