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.

Sign up for GitHub

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

Jump to bottom

feat(Select): Support disabling individual items #1493

Merged
merged 3 commits into from
Dec 16, 2024
Merged

feat(Select): Support disabling individual items #1493

merged 3 commits into from
Dec 16, 2024

Conversation

moathabuhamad-cengage
Copy link
Collaborator

Issue: #1217

What I did

  • added support for disabling individual items in react-magma-dom
  • added a unit test for that behavior
  • added an example in the docs under api/select#disabled_items

Screenshots:

image

Checklist

  • changeset has been added
  • Pull request description is descriptive
  • I have made corresponding changes to the documentation
  • New and existing unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works

How to test

check behavior in storybooks and in docs under api/select#disabled_items

@moathabuhamad-cengage moathabuhamad-cengage added the ready for review Pull requests that are ready for developer review label Oct 16, 2024
@moathabuhamad-cengage moathabuhamad-cengage self-assigned this Oct 16, 2024
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Oct 16, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: 0049174

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
react-magma-dom Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@moathabuhamad-cengage moathabuhamad-cengage linked an issue Oct 16, 2024 that may be closed by this pull request
Select: Support disabling individual items #1217
Closed
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 911c40d to 42a91f7 Compare October 21, 2024 10:22
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@silvalaura
Copy link
Collaborator

I noticed a little bit of a strange behavior with the focus state. In the screenshot below, I was hovering over Blue, and the blue focus border kept appearing for the item below:
image

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 42a91f7 to 31d8c13 Compare October 22, 2024 09:53
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

silvalaura
silvalaura reviewed Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@silvalaura silvalaura left a comment

Choose a reason for hiding this comment

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

Can we add an example with multiple disabled items? Can we also add some disabled items to the Multi example? Also, we need unit tests for multi select.

What happens if initialSelectedItem/initialSelectedItems is an item that is disabled? What if it's initially selected + disabled + isClearable is on?

packages/react-magma-dom/src/components/Select/shared.ts Outdated Show resolved Hide resolved
@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 31d8c13 to 48b4fa8 Compare October 28, 2024 15:01
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@silvalaura
Copy link
Collaborator

I was able to select disabled items:
image
https://jam.dev/c/181a698f-8a4e-4604-a764-53c7a917b634

silvalaura
silvalaura reviewed Oct 28, 2024
View reviewed changes
Copy link
Collaborator

@silvalaura silvalaura left a comment

Choose a reason for hiding this comment

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

A change somewhere here made us lose the focus back on the select input after isClearable X is pressed like we currently have: https://storybook-preview-dev--upbeat-sinoussi-f675aa.netlify.app/?path=/story/select--default&args=isClearable:true

packages/react-magma-dom/src/components/Select/Select.stories.tsx Outdated Show resolved Hide resolved
packages/react-magma-dom/src/components/Select/Select.tsx Show resolved Hide resolved
@moathabuhamad-cengage moathabuhamad-cengage removed the ready for review Pull requests that are ready for developer review label Oct 30, 2024
@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 48b4fa8 to c2b8af1 Compare November 4, 2024 09:14
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 4, 2024

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from c2b8af1 to 7244c63 Compare November 4, 2024 10:31
@moathabuhamad-cengage
Copy link
Collaborator Author

I was able to select disabled items: image https://jam.dev/c/181a698f-8a4e-4604-a764-53c7a917b634

I wasn't able to reproduce this bug but i added more checks anyway

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 89eb6e1 to 595f69c Compare November 5, 2024 15:35
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from 595f69c to bcc3d6d Compare November 5, 2024 16:18
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 5, 2024

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@silvalaura
Copy link
Collaborator

Issues:

  1. Single select > after red is selected, if isClearable is clicked, focus goes to disabled item: https://jam.dev/c/221ef11c-aa53-4484-94ee-da3c6fd428bc
  2. Not sure that initialHighlightedIndex is now working. I change it in storybook and I don't see it affecting the first item that gets focus

@moathabuhamad-cengage moathabuhamad-cengage removed the ready for review Pull requests that are ready for developer review label Nov 6, 2024
@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from bcc3d6d to 1e6333a Compare November 25, 2024 13:19
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@moathabuhamad-cengage moathabuhamad-cengage force-pushed the feat/select-support-disabled-items branch from e8c5b20 to 0049174 Compare November 25, 2024 14:10
@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://docs-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@github-actions GitHub Actions
Copy link
Contributor

🚀 Deployed on https://storybook-preview-1493--upbeat-sinoussi-f675aa.netlify.app

@moathabuhamad-cengage moathabuhamad-cengage added the ready for review Pull requests that are ready for developer review label Nov 25, 2024
@silvalaura silvalaura added needs design feedback Waiting for Design approval or feedback do not merge Pull requests that should not be merged and removed ready for review Pull requests that are ready for developer review labels Nov 25, 2024
@orion-cengage orion-cengage removed the needs design feedback Waiting for Design approval or feedback label Dec 4, 2024
@silvalaura silvalaura removed the do not merge Pull requests that should not be merged label Dec 16, 2024
@silvalaura silvalaura merged commit 79435c9 into dev Dec 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silvalaura silvalaura silvalaura approved these changes

@orion-cengage orion-cengage orion-cengage approved these changes

Assignees

@moathabuhamad-cengage moathabuhamad-cengage

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Select: Support disabling individual items
3 participants
@moathabuhamad-cengage @silvalaura @orion-cengage