-
Notifications
You must be signed in to change notification settings - Fork 233
docs(switch,radio,checkbox,fieldgroup): component analysis migration roadmap #5729
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
base: 2nd-gen-component-analysis
Are you sure you want to change the base?
docs(switch,radio,checkbox,fieldgroup): component analysis migration roadmap #5729
Conversation
|
📚 Branch Preview🔍 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
Tachometer resultsCurrently, no packages are changed by this PR... |
0d65ef2
to
8178de2
Compare
aa004aa
to
a62d03f
Compare
a62d03f
to
68b3d3a
Compare
| | `name` attribute | Missing from CSS | | ||
| | `tabindex` attribute | Missing from CSS | | ||
| | `autofocus` attribute | Missing from CSS | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these something we think is worth mentioning? Would CSS ever come back and implement these attributes?
### CSS => SWC implementation gaps | ||
|
||
**New for S2:** | ||
The checkbox component in Spectrum 2 has the new down state (active) perspective shift applied. Additionally, the checkbox CSS was expanded to ensure coverage of a variety of interactive and variant states: hover, focus-visible, hover+disabled, checked+disabled, focus-visible+checked+hover, indeterminate+invalid, invalid+focused, disabled+read-only, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the down state was the "biggest" thing that was added to checkbox. If I remember correctly, there's some open questions still about whether or not the invalid checkbox has a red border, or if we rely on negative help text only.
Anyways, is the down state and expansion of testing worth mentioning at all? The down state is mainly CSS, but we do use JS to calculate those properties for some components. The testing bit...I think I'm on the side of removing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's definitely mention invalid checkbox as a consideration, even if it ends up not changing, it's probably valuable to call out as we migrate, especially if we work in squads where design is involved as with app frame side nav.
Otherwise, I think the CSS-specific stuff (notes about improved test coverage, things that are present in SWC but missing in CSS) aren't really very valuable, and they probably wouldn't be missed if taken out.
| `.spectrum-Radio-label` | Default slot content | Implemented | | ||
| `.spectrum-Radio-label:lang(ja)`, `.spectrum-Radio-label:lang(ko)`, `.spectrum-Radio-label:lang(zh)` | Language-specific styling | Implemented | | ||
| `.spectrum-Radio.is-readOnly` | `readonly` attribute | Implemented | | ||
| | `invalid` attribute | Missing from CSS | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to the checkbox, where I think instead of putting invalid
on the radio, we pass that arg/property to the help text instead. So radio itself doesn't really have an invalid state- the radio group has an invalid state that is shown via negative help text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty straightforward set of components! Just a few tiny comments here and then I think this one will be good to go!
### CSS => SWC implementation gaps | ||
|
||
**New for S2:** | ||
The checkbox component in Spectrum 2 has the new down state (active) perspective shift applied. Additionally, the checkbox CSS was expanded to ensure coverage of a variety of interactive and variant states: hover, focus-visible, hover+disabled, checked+disabled, focus-visible+checked+hover, indeterminate+invalid, invalid+focused, disabled+read-only, etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's definitely mention invalid checkbox as a consideration, even if it ends up not changing, it's probably valuable to call out as we migrate, especially if we work in squads where design is involved as with app frame side nav.
Otherwise, I think the CSS-specific stuff (notes about improved test coverage, things that are present in SWC but missing in CSS) aren't really very valuable, and they probably wouldn't be missed if taken out.
<div class="group" role="presentation"> | ||
<slot @slotchange="handleSlotchange"></slot> | ||
</div> | ||
<!-- Help text rendered via renderHelpText() method --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the way this is done!
| `.spectrum-FieldGroup--sidelabel` | Layout variant | Missing from WC | | ||
| `.spectrum-FieldGroup--toplabel` | Layout variant | Missing from WC | | ||
| `.spectrum-FieldGroupInputLayout` | Internal layout container | Missing from WC | | ||
| `.spectrum-FieldGroup-item` | Individual field item classes | Missing from WC | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been leaving the comment on all the PRs that we should include some kind of quick comment on why it's missing from WC like
| `.spectrum-FieldGroup-item` | Individual field item classes | Missing from WC | | |
| Missing from WC (new for Spectrum 2) | |
But I feel like in this case it's a little bit more complicated than that. The side label and top label I think is more of an implementation gap and we could note that, but for the layout classes, I can't think of a quick way to explain that in the table.
- `.spectrum-FieldGroupInputLayout` wrapper container is not generated by the web component. | ||
- `.spectrum-FieldGroup-item` classes are not automatically applied to slotted elements. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just a smidge more explanation on why SWC might want to incorporate these, I'm assuming we have these to make the layout work properly.
Description
Creates AI-generated migration documentation to analyze component differences to guide SWC migration to S2, with human vetting. The documentation serves as a bridge between the migrated Spectrum 2 CSS work and the corresponding web components, in order to help engineers understand what needs to be implemented, updated, or aligned between the two systems to guide the development of 2nd generation web components.
This batch is for the following components: Checkbox, Radio, Switch, Field group
Motivation and context
Related issue(s)
SWC-1217
Screenshots (if appropriate)
Author's checklist
I have added automated tests to cover my changes.I have included a well-written changeset if my change needs to be published.Reviewer's checklist
Includes thoughtfully written changeset if changes suggested includepatch
,minor
, ormajor
featuresAutomated tests cover all use cases and follow best practices for writingValidated on all supported browsersAll VRTs are approved before the author can update Golden HashManual review test cases
Testing Checklist
Documentation Quality
Cross-Reference Accuracy
metadata.json
files for S2