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-group): value handling #33362

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

fix(radio-group): value handling #33362

wants to merge 10 commits into from

Conversation

olaf-k
Copy link

@olaf-k olaf-k commented Nov 27, 2024

This short PR attempts to fix the radio-group's value setting.

In the current code, inferring a radio-group's initial value from a checked radio child does not work properly:

  • the form's value is not set
  • the radio-group is not focusable (because all its radios get -1 tabindex)

Both of these behaviors stem from the fact that the value setter's logic is not called. This PR corrects that by triggering it in the slot change handler.

I've also taken the opportunity to:

  • remove Updates.enqueue from this handler as it defers the component initialization (first call to radiosChanged) to the next event loop tick, which is not great UX if authors want to get the value early (e.g., in a whenDefined call).
  • replace direct calls to checkRadio() by radio.click() in radio-group so that keyboard navigation would also emit the proper events.
  • fix the default validation messages copied from the native element (radio had the one from checkbox and radio-group lacked the mandatory name prop).
  • make said default validation message static.

(Not related but, for some reason, there is code that unchecks a radio when it becomes disabled. I'm not sure that's wanted as it is a strange departure from native behavior, so I also removed that.)
I reverted the "disabled unchecked" change as I learned the hard way the Fluent component is designed to work this way. (I rewrote the logic for my own component, let me know if you're interested in another PR for that).

@olaf-k olaf-k marked this pull request as ready for review November 27, 2024 16:48
@olaf-k olaf-k requested a review from a team as a code owner November 27, 2024 16:48
@chrisdholt chrisdholt requested a review from radium-v November 27, 2024 16:50
@chrisdholt
Copy link
Member

Adding @radium-v to review, thanks @olaf-k!

Copy link

github-actions bot commented Nov 27, 2024

📊 Bundle size report

✅ No changes found

Copy link

Pull request demo site: URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants