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(radio): fix radio not showing checked state when not in a group #24423

Merged
merged 4 commits into from
Dec 16, 2021

Conversation

averyjohnston
Copy link
Contributor

@averyjohnston averyjohnston commented Dec 15, 2021

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Issue Number: #24291

If an ion-radio is not part of an ion-radio-group, clicking it does not apply the radio-checked class, making it look like nothing happened. However, the native input does get correctly set to checked=true.

This bug looks to be masquerading as the one described in the issue linked above. While having multiple inputs in an ion-item and disabling one of them does not currently disable the other inputs, using an ion-radio as the enabled input can make it look that way.

What is the new behavior?

The checked property of the ion-radio component is updated on click to match the native input. This is then picked up by the existing @Watch('checked'), which applies the proper styling.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build posted on the original issue to be definitively sure that the source was this bug.

Copy link
Contributor

@sean-perkins sean-perkins 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 👍

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Does the issue reported in #24291 only trigger with the use of ion-radio? If so, I'd like to discuss whether using ion-radio without a group is a use case we should support.

The docs currently state that radios should be used inside of ion-radio-group: https://ionicframework.com/docs/api/radio

@averyjohnston
Copy link
Contributor Author

@liamdebeasi As far as I can tell, yes. I posted a dev build for the OP to try out in case there's an edge case I missed, but I tried every combination I could think of and only saw the issue described in this PR.

Our tests do have items for radios outside groups, so I figured that meant they were supported 🤔 There's also code in the component specifically to check whether it's in a group.

Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

The best practice right now is to use radios in a group, but even the native radio input works as a standalone, so I think this is a good fix to merge to better align with the native web behavior. Great job!

@averyjohnston averyjohnston merged commit 94a781c into main Dec 16, 2021
@averyjohnston averyjohnston deleted the FW-378 branch December 16, 2021 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants