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

fix: radio button storybook bug #16082

Merged
merged 6 commits into from Apr 3, 2024
Merged

fix: radio button storybook bug #16082

merged 6 commits into from Apr 3, 2024

Conversation

andreancardona
Copy link
Contributor

@andreancardona andreancardona commented Mar 27, 2024 •
edited

Closes #15705

  • added the defaultSelected prop in the RadioButton playground
  • updated docs to better support the need for RadioButton (s) to be rendered as children of RadioButtonGroup

Changelog

Removed

Removed the following props from RadioButton

  • checked
  • defaultChecked

Testing / Reviewing

  • Checkout the defaultSelected prop in the RadioButton playground

@andreancardona andreancardona requested a review from a team as a code owner March 27, 2024 18:55
@netlify Netlify
Copy link

netlify bot commented Mar 27, 2024

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 9d0afa6
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66046bad82ffed00085ff2ae
😎 Deploy Preview https://deploy-preview-16082--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.

alisonjoseph
alisonjoseph requested changes Mar 27, 2024
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Looking at the issue, looks like we also need to remove the defaultSelected prop from the default story.

@netlify Netlify
Copy link

netlify bot commented Mar 27, 2024 •
edited

✅ Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit a3522a6
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/660d5c914a14010008196e46
😎 Deploy Preview https://deploy-preview-16082--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.

tay1orjones
tay1orjones approved these changes Mar 27, 2024
View reviewed changes
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.

Yeah, I agree with Alison. Other than that I think this looks great

@andreancardona
Copy link
Contributor Author

andreancardona commented Mar 27, 2024 •
edited

@alisonjoseph it shouldn't be selected - is that what you mean? Just want to clarify here:

Screenshot 2024-03-27 at 15 09 19

alisonjoseph
alisonjoseph requested changes Mar 29, 2024
View reviewed changes
Copy link
Member

@alisonjoseph alisonjoseph left a comment •
edited

Choose a reason for hiding this comment

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

oops hit approve accidentally. The issue says not selected. I think if you just remove defaultChecked from the story that should work.

Screenshot 2024-03-29 at 8 26 11 AM

Kritvi-bhatia17
Kritvi-bhatia17 previously requested changes Apr 2, 2024
View reviewed changes
Copy link

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

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

Hi @andreancardona, I noticed the smart adjustment you made for the default dropdown options!
However, I'm encountering an issue where the default dropdown options 1, 2, and 3 aren't functioning as expected. When I select different options, the output consistently shows the 3rd option as selected. Could you please take a look?

Screen.Recording.2024-04-02.at.1.28.05.PM.mov

@tay1orjones
Copy link
Member

@Kritvi-bhatia17 defaultSelected only impacts which one is selected in the initial render. To see your change you'll need to refresh the story

defaults.update.after.refresh.mov

@Kritvi-bhatia17
Copy link

@Kritvi-bhatia17 defaultSelected only impacts which one is selected in the initial render. To see your change you'll need to refresh the story

defaults.update.after.refresh.mov

Ohh damn, I didn't know that earlier. Thanks Taylor!

Kritvi-bhatia17
Kritvi-bhatia17 approved these changes Apr 3, 2024
View reviewed changes
Copy link

@Kritvi-bhatia17 Kritvi-bhatia17 left a comment

Choose a reason for hiding this comment

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

Nice work @andreancardona 🔥
Looks good to me from design perspective!

tw15egan
tw15egan approved these changes Apr 3, 2024
View reviewed changes
Copy link
Member

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@tw15egan tw15egan enabled auto-merge April 3, 2024 13:41
@tw15egan tw15egan added this pull request to the merge queue Apr 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 3, 2024
@andreancardona andreancardona added this pull request to the merge queue Apr 3, 2024
Merged via the queue into carbon-design-system:main with commit f28b9ab Apr 3, 2024
20 checks passed
@andreancardona andreancardona deleted the 15705-radio-button-bug branch April 3, 2024 16:36
preetibansalui pushed a commit to tay1orjones/carbon that referenced this pull request Apr 24, 2024
@andreancardona @tw15egan @preetibansalui
fix: radio button storybook bug (carbon-design-system#16082)
660da25 
* fix: radio button storybook bug

* Update packages/react/src/components/RadioButton/RadioButton.stories.js

* fix: remove default selected

* fix: update tests

---------

Co-authored-by: TJ Egan <tw15egan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alisonjoseph alisonjoseph alisonjoseph approved these changes

@tay1orjones tay1orjones tay1orjones approved these changes

@tw15egan tw15egan tw15egan approved these changes

@Kritvi-bhatia17 Kritvi-bhatia17 Kritvi-bhatia17 approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Selectable Components: Radio Button] Dev Implementation
5 participants
@andreancardona @tay1orjones @Kritvi-bhatia17 @alisonjoseph @tw15egan