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(SegmentedControls): allow for item in list to be disabled #1676

Merged
merged 8 commits into from
Mar 5, 2024

Conversation

ryancarville
Copy link
Contributor

@ryancarville ryancarville commented Dec 13, 2023

ClickUp Report

Report: Request to disable single items in a group of items

Solution:

  1. Add a disabled prop to the item type.
  2. Update the map of the item component to reference item.disabled || disabled (default to the group disabled prop)
  3. Update the selectedIndex method to by-pass the item if it is disabled (this is the key index that sets the activeBorder motion div)
  4. Move the motion div outside the fieldset and update the styles accordingly. The is required to allow the border to be the proper z-index else the disabled background will overlap the left border edge. Added bonus is it is much more responsive to the correct sizing.

@ryancarville ryancarville marked this pull request as ready for review December 13, 2023 10:28
Copy link

netlify bot commented Dec 13, 2023

Deploy Preview for beta-fondue-components ready!

Name Link
🔨 Latest commit 8a572f5
🔍 Latest deploy log https://app.netlify.com/sites/beta-fondue-components/deploys/6579b784dac52a000826f823
😎 Deploy Preview https://deploy-preview-1676--beta-fondue-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.

@ryancarville ryancarville requested a review from a team December 13, 2023 11:47
@ryancarville ryancarville self-assigned this Dec 14, 2023
@ryancarville ryancarville added the enhancement New feature or request label Dec 14, 2023
@noahwaldner noahwaldner changed the base branch from beta to main February 21, 2024 12:21
Copy link

changeset-bot bot commented Feb 21, 2024

⚠️ No Changeset found

Latest commit: fd8c9ef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

Copy link

netlify bot commented Feb 21, 2024

Deploy Preview for beta-fondue-components ready!

Name Link
🔨 Latest commit fd8c9ef
🔍 Latest deploy log https://app.netlify.com/sites/beta-fondue-components/deploys/65e5d77ed84e6600094a0b06
😎 Deploy Preview https://deploy-preview-1676--beta-fondue-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.

Copy link

netlify bot commented Feb 21, 2024

Deploy Preview for fondue-components ready!

Name Link
🔨 Latest commit fd8c9ef
🔍 Latest deploy log https://app.netlify.com/sites/fondue-components/deploys/65e5d77ecedd8b0008401a02
😎 Deploy Preview https://deploy-preview-1676--fondue-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.

Comment on lines 161 to 162
const nextIndex = index + 1;
radioGroupState.setSelectedValue(items[nextIndex].id);
Copy link
Member

@SamuelAlev SamuelAlev Feb 21, 2024

Choose a reason for hiding this comment

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

Goes out of bounds in case of the last item being disabled, also a disabled item should be able to be selected by default which makes the change in this function not necessary 🤔
What do you think?

Copy link
Contributor

@noahwaldner noahwaldner Mar 1, 2024

Choose a reason for hiding this comment

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

I had a look at it and it is very weird when the default item is not clickable. The UX is super weird.

I fixed the function to select the item to the left when the last one is default and disabled and to collapse the "cursor/hightlight" when all are disabled

@noahwaldner noahwaldner requested a review from SamuelAlev March 1, 2024 13:19
@noahwaldner noahwaldner merged commit d9fc188 into main Mar 5, 2024
23 checks passed
@noahwaldner noahwaldner deleted the fix/segmented-controls-item-disabled branch March 5, 2024 10:13
Copy link
Contributor

github-actions bot commented Mar 5, 2024

Lead time: 82 days, 23 hours, 53 minutes, 14 seconds (1991.89 total hours) from first commit to close.
Review time: 82 days, 23 hours, 44 minutes, 32 seconds (1991.74 total hours) from ready for review to close.

  • First commit: 13.12.2023, 11:20:17.
  • Ready for review: 13.12.2023, 11:28:59.
  • Closed: 5.3.2024, 11:13:31.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants