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

[Docs] Fix the controlled RadioGroup example #6057

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented Mar 14, 2024

Closes #6056

Hey guys I have one question.

Would it be better to just add the empty string ('') check over here?

let tabIndex: number | undefined = -1;
if (state.selectedValue != null) {
if (state.selectedValue === value) {
tabIndex = 0;
}
} else if (state.lastFocusedValue === value || state.lastFocusedValue == null) {
tabIndex = 0;
}
if (isDisabled) {
tabIndex = undefined;
}

to something like this?

let tabIndex: number | undefined = -1;
- if (state.selectedValue != null) {
+ if (state.selectedValue != null && state.selectedValue !== '') {
  if (state.selectedValue === value) {
    tabIndex = 0;
  }
} else if (state.lastFocusedValue === value || state.lastFocusedValue == null) {
  tabIndex = 0;
}
if (isDisabled) {
  tabIndex = undefined;
}

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Docs

http://localhost:1234/react-aria/useRadioGroup.html#controlled-value

radio_controlled_keyboard.mov

🧢 Your Project:

@snowystinger
Copy link
Member

GET_BUILD

@snowystinger
Copy link
Member

Thanks @sookmax this is the correct fix, we allow falsy id's such as empty strings. So you could do something like this https://stackblitz.com/edit/vitejs-vite-28q1zf?file=src%2FApp.tsx,package.json

@rspbot
Copy link

rspbot commented Mar 15, 2024

@rspbot
Copy link

rspbot commented Mar 15, 2024

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@sookmax
Copy link
Contributor Author

sookmax commented Mar 15, 2024

@snowystinger Ah that's interesting! Never imagined there would be a case where a radio value actually is "" haha. Thanks for sharing the sandbox!

@snowystinger snowystinger merged commit 449bc07 into adobe:main Mar 15, 2024
27 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.

React-Aria useRadioGroup hook controlled value example not keyboard accessible
4 participants