-
Notifications
You must be signed in to change notification settings - Fork 233
chore(docs): 2nd gen component analysis for avatar, opacity checkerboard, swatch + swatchgroup, thumbnail #5740
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?
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... |
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.
Left some comments! We'll definitely want to fill in the Resources sections here, plus review some of the callouts that the docs are making for accuracy!
152b6af
to
c5f8345
Compare
```html | ||
<!-- With link --> | ||
<a id="link" class="link" href="[href]"> | ||
<img class="image" alt="[label]" src="[src]" /> |
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.
oh I love how this showcases where the props go!
migration-roadmap/avatar.md
Outdated
| `.spectrum-Avatar-image` | `src` attribute | Implemented | | ||
| `.spectrum-Avatar-link` | `href` attribute | Implemented | | ||
| `.is-focused` | Focus state | Implemented | | ||
| `.is-disabled` | `disabled` attribute | 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.
oh yeah, I think Rise mentioned we could mark the disabled attribute as Implemented
.
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.
Nice, I'll update this
|
||
## Summary of changes | ||
|
||
### CSS => SWC implementation gaps |
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.
oh! There's a follow up ticket for avatar with some newly added default gradient background support. Should we call out that there's more to this component that isn't supported by CSS yet either?
Just thinking out loud- when would that work be done? Separately from the SWC migration to S2? Like a follow up once avatar has migrated?
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.
Oh dang, that's tough. I'm tempted to call it "out of scope" for now since it wasn't in place when we first migrated the component. Do you think we're likely to miss it if we do that though?
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 don't think it hurts to include information about variants that we didn't implement in CSS, and I'm not sure what the exact order of implementation would be, but I would guess that we might do a single pass to bring our existing CSS over, then go back and address the other variants later, so it might not be absolutely necessary to include for the "first pass" migration.
We do have a ticket for those additional variants: CSS-1236. If that ends up happening in CSS before we migrate, we should come back and note it in these docs. But if we address it in SWC, I think we'll be ok.
I'm also hoping/assuming that we'll do some kind of design review before shipping anything so that's another fallback for missing variants to be called out!
|
||
```html | ||
<!-- Used as a utility class in other components --> | ||
<div class="opacity-checkerboard [variant]"> |
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.
What would one of the variant options be? Like fill
(which I saw in the swatch story)?
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.
Oh yeah, I suppose it doesn't have any variants. I can remove this reference.
|
||
### CSS Spectrum 2 changes | ||
|
||
No significant structural changes between CSS main and spectrum-two branches. The templates are identical, indicating that the swatch group component structure remains consistent across Spectrum 2 migration. Swatch group includes a setting for single or multiple selection and it might be worth connecting with design to see if there should be any unique styles designed that differentiates single or multi-select swatch groups. |
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'm not sure where this information would be most helpful, but could we add a short description of why the border prop for swatch group is getting deprecated for SWC S2?
If I'm remembering correctly, the border used to be applied per-swatch within a swatch group (as in- low-contrast swatches would get the light border, while higher contrast swatches didn't need a border). Design decided that they wanted borders on all swatches for S2, however. The only exception looks like the addSwatch.
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 added some verbage to this end, let me know if it sounds okay!
|
||
<!-- With background --> | ||
<div | ||
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
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.
Is this background example supposed to have the layer class still?
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.
🤔
|
||
<!-- Default --> | ||
<div | ||
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
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.
same here- I assume the layer class needs to be removed.
<div | ||
class="spectrum-Thumbnail-background" | ||
style="background-color: [backgroundColor]" | ||
></div> |
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.
And then this gets removed from the Default example markup too, I assume?
|
||
**CSS features missing from Web Component:** | ||
|
||
- Disabled state support (`.is-disabled`) |
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 disabled is supported in SWC! It's not on the API table, though.
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 wonder how that happened... it looks like focus and selected are supported as well.
|
||
- [CSS migration](https://github.com/adobe/spectrum-css/pull/3394) | ||
- [Spectrum 2 preview](https://spectrumcss.z13.web.core.windows.net/pr-2352/index.html?path=/docs/components-opacity-checkerboard--docs) | ||
- [React](https://react-spectrum.adobe.com/s2/index.html?path=/docs/colorslider--docs) (not a standalone component, but styles are used by the color slider) |
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.
not a standalone component, but styles are used by the color slider
Love this clarification, will be borrowing this for other components if it comes up.
migration-roadmap/swatch.md
Outdated
|
||
### CSS Spectrum 2 changes | ||
|
||
There is a new add swatch functionality in the `spectrum-two` branch and supplemental state and pseudo-element styling selectors in the `spectrum-two` branch which will automatically be applied when consumed by the web component for Spectrum 2. |
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.
Just a nit to make the name of the feature stand out more. This seems to align with Adobe's grammar & mechanics guidelines although I do see other examples on the page where they're using quotes or italics.
There is a new add swatch functionality in the `spectrum-two` branch and supplemental state and pseudo-element styling selectors in the `spectrum-two` branch which will automatically be applied when consumed by the web component for Spectrum 2. | |
There is a new **add swatch** functionality in the `spectrum-two` branch and supplemental state and pseudo-element styling selectors in the `spectrum-two` branch which will automatically be applied when consumed by the web component for Spectrum 2. |
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.
Yes, love it!
<details> | ||
<summary>Attributes</summary> | ||
|
||
- `border` (string) - Border style: 'light', 'none' **deprecated** (from Swatch component) |
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.
Yep, as mentioned with Swatch the borders are more transparent when in the group.
|
||
<!-- With background --> | ||
<div | ||
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
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.
🤔
| `.is-disabled` | `disabled` attribute | Missing from WC | | ||
| `.is-focused` | Focus state | Missing from WC | | ||
| `.is-selected` | `selected` attribute | 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.
It looks like these states do exist in SWC
|
||
**CSS features missing from Web Component:** | ||
|
||
- Disabled state support (`.is-disabled`) |
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 wonder how that happened... it looks like focus and selected are supported as well.
1c7d103
to
ab5ae6a
Compare
c5f8345
to
088c63c
Compare
088c63c
to
ee5fdd5
Compare
|
||
**CSS features missing from Web Component:** | ||
|
||
- Disabled state support (`.is-disabled`) |
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.
- Disabled state support (`.is-disabled`) |
|
||
## Summary of changes | ||
|
||
### CSS => SWC implementation gaps |
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 don't think it hurts to include information about variants that we didn't implement in CSS, and I'm not sure what the exact order of implementation would be, but I would guess that we might do a single pass to bring our existing CSS over, then go back and address the other variants later, so it might not be absolutely necessary to include for the "first pass" migration.
We do have a ticket for those additional variants: CSS-1236. If that ends up happening in CSS before we migrate, we should come back and note it in these docs. But if we address it in SWC, I think we'll be ok.
I'm also hoping/assuming that we'll do some kind of design review before shipping anything so that's another fallback for missing variants to be called out!
- `.spectrum-Swatch-disabledIcon path:last-child` | ||
- `.spectrum-Swatch-fill` | ||
- `.spectrum-Swatch-fill:before` | ||
- `.spectrum-Swatch-icon` |
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.
Duplicate:
- `.spectrum-Swatch-icon` |
- `.spectrum-Swatch-fill` | ||
- `.spectrum-Swatch-fill:before` | ||
- `.spectrum-Swatch-icon` | ||
- `.spectrum-Swatch-image` |
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.
Another duplicate (the suggestion doesn't seem to work for me to suggest removing 2 lines of code, sorry for the duplicate suggestion 😆)
- `.spectrum-Swatch-image` |
- `rounding` (string) - Corner rounding: 'none', 'full' | ||
- `selected` (boolean) - Whether the swatch is selected | ||
- `shape` (string) - Shape variant: 'rectangle' | ||
- `size` (string) - Size: 'xs', 's', 'm', 'l' |
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.
Is it maybe because it's using a size mixin? I've noticed if the types aren't defined within the file (if they're imported) those don't get picked up either.
|
||
<!-- Default --> | ||
<div | ||
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
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.
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" | |
class="spectrum-Thumbnail spectrum-Thumbnail--cover is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
|
||
<!-- With background --> | ||
<div | ||
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
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.
class="spectrum-Thumbnail spectrum-Thumbnail--cover spectrum-Thumbnail-layer is-selected is-disabled is-focused spectrum-Thumbnail--size500" | |
class="spectrum-Thumbnail spectrum-Thumbnail--cover is-selected is-disabled is-focused spectrum-Thumbnail--size500" |
<div | ||
class="spectrum-Thumbnail-background" | ||
style="background-color: [backgroundColor]" | ||
></div> |
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.
As with legacy, the default thumbnail doesn't have this background div
<div | |
class="spectrum-Thumbnail-background" | |
style="background-color: [backgroundColor]" | |
></div> |
| `.is-disabled` | `disabled` attribute | Missing from WC | | ||
| `.is-focused` | Focus state | Missing from WC | | ||
| `.is-selected` | `selected` attribute | 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.
| `.is-disabled` | `disabled` attribute | Missing from WC | | |
| `.is-focused` | Focus state | Missing from WC | | |
| `.is-selected` | `selected` attribute | Missing from WC | | |
| `.is-disabled` | `disabled` attribute | Implemented | | |
| `.is-focused` | Focus state | Implemented | | |
| `.is-selected` | `selected` attribute | Implemented | |
- Disabled state support (`.is-disabled`) | ||
- Focus state support (`.is-focused`) | ||
- Selected state support (`.is-selected`) |
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 focus, selected, and disabled, are all there in SWC.
- Disabled state support (`.is-disabled`) | |
- Focus state support (`.is-focused`) | |
- Selected state support (`.is-selected`) | |
None found for this component. |
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 barebones components:
Motivation and context
Related issue(s)
SWC-1221
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 HashDocumentation Quality
Cross-Reference Accuracy
metadata.json
files