fix(radio): remove deprecated invalid property in favor of group validation#5958
fix(radio): remove deprecated invalid property in favor of group validation#5958JinMokai wants to merge 12 commits intoadobe:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 1223741 The changes in this PR will be included in the next version bump. This PR includes changesets to release 84 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Hello reviewers, greetings to you. I have preliminarily completed what I believe needs to be done. If there are any errors or irregularities in this PR, please point them out, and I will address the issues and respond as soon as possible. Thank you once again!😊😊 |
blunteshwar
left a comment
blunteshwar
left a comment
There was a problem hiding this comment.
After removing
@property({ type: Boolean, reflect: true }) public invalid = false;
I don't see the property being added/declared in radio-group
rubencarvalho
left a comment
rubencarvalho
left a comment
There was a problem hiding this comment.
Hey Jin! Thanks for the contribution. It is looking great!
Unfortunately, due to our semver policy, we can't remove the invalid property from sp-radio without a major version bump, which we can't do right now.
I do see a path forward here, though:
What if we have sp-radio-group observe invalid attribute changes on its child sp-radio elements and sync its own accessible invalid state accordingly, plus show a dev deprecation warning guiding users to the correct usage. This maintains backward compatibility while fixing the A11Y bug by centralizing the invalid state on the parent.
That said, if you have another approach, we're open to it! 😄
Thanks again for your contribution!
The sp-radio-group now manages the invalid state based on its child elements. The standalone invalid property of sp-radio has been deprecated.
|
hi, @rubencarvalho @blunteshwar |
| }); | ||
|
|
||
| it('warns when [invalid] is used on children and updates group invalid state', async () => { | ||
| const consoleWarnSpy = spy(console, 'warn'); |
There was a problem hiding this comment.
RadioGroup uses window.__swc.warn(...) but you are using spy(console.warn). If window.__swc.warn doesn't call console.warn in the test environment, tests will fail or become flaky.
- Keep `invalid` property on sp-radio for backward compatibility (marked @deprecated) - RadioGroup now observes and syncs invalid state from child radios - Add deprecation warning to guide users toward group-level validation - Prevent clearing user-set invalid states with _managedInvalid flag
|
hi @Rajdeepc
|
caseyisonit
added
the
Status:Addressing feedback
5 participants
fix(radio): remove deprecated invalid property in favor of group validation
Description
Removed the deprecated
invalidproperty from thesp-radiocomponent. Validation state should be managed at thesp-radio-grouplevel.invalidproperty andaria-invalidhandling fromsp-radio.sp-radio-groupto reflect theinvalidstate using thearia-invalidattribute.invalidcontrols on individual radio buttons.aria-invalidtoggling onsp-radio-group.Motivation and context
The
invalidproperty on individualsp-radioelements was deprecated. Standard accessibility and design patterns for radio buttons dictate that validation should occur at the group level rather than on individual options. This change enforces that pattern and ensures thearia-invalidstate is correctly applied to the group container.This is my first contribution to the SWC components. My motivation is to learn more about accessibility knowledge, nothing else, just passion!
Related issue(s)
Screenshots (if appropriate)
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Descriptive Test Statement
Descriptive Test Statement
Device review