-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: filterable multiselect readonly state implemented #17662
base: main
Are you sure you want to change the base?
fix: filterable multiselect readonly state implemented #17662
Conversation
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
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, some minor changes only.
packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/components/MultiSelect/FilterableMultiSelect.tsx
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17662 +/- ##
==========================================
+ Coverage 77.07% 78.94% +1.86%
==========================================
Files 409 408 -1
Lines 14021 14034 +13
Branches 4355 4356 +1
==========================================
+ Hits 10807 11079 +272
+ Misses 3043 2788 -255
+ Partials 171 167 -4 ☔ View full report in Codecov by Sentry. |
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.
Thanks Guru for the changes, Sorry one minor change again. Otherwise it looks good to me.
@@ -728,7 +737,7 @@ const FilterableMultiSelect = React.forwardRef(function FilterableMultiSelect< | |||
placeholder, | |||
preventKeyAction: isOpen, | |||
|
|||
onClick: () => handleMenuChange(true), | |||
onClick: () => !readOnly && handleMenuChange(true), |
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.
We are checking readOnly
condition here and in handleMenuChange
function both. I don't think it's necessary to have it in both places, as one check would also work same.
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.
Thanks for the changes Guru, Looks Good now.
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 to me 👍
@laurenmrice could you do a review as well? Just want to be sure we didn't miss anything design-wise
Closes #17587
read only state for filterable multiselect is implemented
Changelog
New
readOnly
propreadOnlyEventHandlers
method to handle events during thereadOnly
state.Changed
Testing / Reviewing
readOnly
prop and see if it works as expected.