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

fix: filterable multiselect readonly state implemented #17662

Conversation

Gururajj77
Copy link
Contributor

Closes #17587

read only state for filterable multiselect is implemented

Changelog

New

  • added new readOnly prop
  • new readOnlyEventHandlers method to handle events during the readOnly state.

Changed

  • changes related to readOnly state

Testing / Reviewing

  • Go to the 'Filterable' story in the Multiselect component in the deploy preview
  • Turn on the readOnly prop and see if it works as expected.

Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit dea3128
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/67124117375ab30008bae07a
😎 Deploy Preview https://deploy-preview-17662--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Oct 7, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit dea3128
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/67124117b4826e00087d69bf
😎 Deploy Preview https://deploy-preview-17662--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@preetibansalui preetibansalui left a 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.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.70%. Comparing base (4312ae4) to head (dea3128).

Files with missing lines Patch % Lines
...c/components/MultiSelect/FilterableMultiSelect.tsx 52.94% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #17662      +/-   ##
==========================================
- Coverage   79.72%   79.70%   -0.02%     
==========================================
  Files         406      406              
  Lines       14012    14030      +18     
  Branches     4333     4364      +31     
==========================================
+ Hits        11171    11183      +12     
- Misses       2676     2681       +5     
- Partials      165      166       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@preetibansalui preetibansalui left a 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.

Copy link
Contributor

@preetibansalui preetibansalui left a 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.

@tay1orjones tay1orjones requested review from a team and laurenmrice and removed request for a team October 11, 2024 17:59
Copy link
Member

@tay1orjones tay1orjones left a 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

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The tag border should be color token $border-subtle. I notice this is also wrong though in the regular Multiselect playground story.
Screenshot 2024-10-14 at 12 11 18 PM
  • When you hover on the x icon in the tag, it turns black and lets you dismiss the tag. The x should not receive any color change or interaction in the read-only state.
    Oct-14-2024 12-15-58

@Gururajj77 Gururajj77 self-assigned this Oct 15, 2024
@Gururajj77
Copy link
Contributor Author

Gururajj77 commented Oct 15, 2024

  • The tag border should be color token $border-subtle. I notice this is also wrong though in the regular Multiselect playground story.
Screenshot 2024-10-14 at 12 11 18 PM

Hey @laurenmrice , I have added the functionality to not have any interaction during the readOnly state in FilterableMultiselect, I'll create new issues for the Multiselect interaction and the $border-subtle mismatch for both components.

Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay thanks, @Gururajj77 ! One last thing I noticed for the Filterable Multiselect is if you hover on the number in the tag in the read-only state, a disabled mouse cursor appears. The mouse cursor should just be the arrow, like how it appears when you hover on the x icon in the tag. I also see the number 2 in the tag looks like it has a highlighted background color behind it which it should not have.

Oct-15-2024 15-02-29

Copy link

netlify bot commented Oct 18, 2024

Deploy Preview for v11-carbon-web-components ready!

Name Link
🔨 Latest commit dea3128
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-web-components/deploys/671241170f9bb700078ee10e
😎 Deploy Preview https://deploy-preview-17662--v11-carbon-web-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Gururajj77
Copy link
Contributor Author

Have created an issue, #17820 to keep track for the hover issue separately

@Gururajj77 Gururajj77 added this pull request to the merge queue Oct 22, 2024
Merged via the queue into carbon-design-system:main with commit 7115f33 Oct 22, 2024
40 checks passed
@Gururajj77 Gururajj77 deleted the filterable-multiselect-readonly-state branch October 22, 2024 09:06
annawen1 pushed a commit to annawen1/carbon that referenced this pull request Oct 22, 2024
…-system#17662)

* fix: filterable multiselect readonly state implemented

* fix: reviewed changes added

* fix: resovled pr review comments

* feat: added readonly state for listboxselection

* fix: removed conditional class on readonly matching disabled

---------

Co-authored-by: Preeti Bansal <[email protected]>
@carbon-automation
Copy link
Contributor

Hey there! v11.69.0 was just released that references this issue/PR.

irfadkp pushed a commit to irfadkp/carbon that referenced this pull request Oct 24, 2024
…-system#17662)

* fix: filterable multiselect readonly state implemented

* fix: reviewed changes added

* fix: resovled pr review comments

* feat: added readonly state for listboxselection

* fix: removed conditional class on readonly matching disabled

---------

Co-authored-by: Preeti Bansal <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[Bug]: Read Only state missing on FilterableMultiselect component
5 participants