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: added controlled logic for switch #297

Merged

Conversation

maxrchung
Copy link
Contributor

@maxrchung maxrchung commented Dec 6, 2023

This PR addresses #296. It doesn't seem like I have permissions to directly link issue.

This change allows EbaySwitch to be controlled by checked prop.

Notes

  • Added similar isControlled logic from EbayCheckbox into EbaySwitch.
  • Added defaultChecked prop to follow logic from EbayCheckbox.

Testing

  • Added unit test to verify checkbox is not changed.
  • Added story that changes checked prop.

Demo

Showing issue before and updated stories after:

Screen.Recording.2023-12-05.at.6.02.29.PM.mov

@maxrchung maxrchung marked this pull request as ready for review December 6, 2023 02:00
Copy link
Member

@HenriqueLimas HenriqueLimas left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. Usually we do this type of condition with defaultChecked and checked arguments. When defaultChecked is passed then it is an uncontrolled component, and when checked is passed that is controlled. We should do the same here for consistency. This will probably be a breaking change though. You can check how we do for ebay-checkbox here

Copy link
Member

@HenriqueLimas HenriqueLimas left a comment

Choose a reason for hiding this comment

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

Thank you for the PR 🎉

@HenriqueLimas
Copy link
Member

We will consider as a patch version since it is aligning the behavior with other components, although it might break some application. We will work with the teams and make the proper changes.

@HenriqueLimas HenriqueLimas merged commit 67da89b into eBay:main Dec 6, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants