docs(menu, action-menu): component analysis#5981
docs(menu, action-menu): component analysis#5981aramos-adobe wants to merge 14 commits into2nd-gen-component-analysisfrom
Conversation
…tton, In-field button and In-field progress circle (#5738) * chore(docs): barebones analysis for SWC-1218 * chore(docs): address feedback * chore(docs): address feedback
…roadmap (#5729) * docs(checkbox): migration roadmap * docs(fieldgroup): migration roadmap * docs(radio): migration roadmap * docs(switch): migration roadmap * docs(checkbox, field-group): update migration roadmap - gives clarification on invalid checkbox consideration - clarifies some of the implementation gaps for field group
… roadmap (#5775) * docs: add component analysis docs adds migration roadmap and component analysis docs for meter, progress bar, slider, illustrated message and drop zone.
…ard, swatch + swatchgroup, thumbnail (#5740)
|
📚 Branch Preview Links🔍 First Generation 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 |
marissahuysentruyt
left a comment
There was a problem hiding this comment.
Oh no! I'm seeing quite a few Cursor hallucinations. Would you double check some things, and maybe re-run the prompt for a few places?
| - [CSS migration]() | ||
| - [Spectrum 2 preview]() | ||
| - [React]() |
There was a problem hiding this comment.
Can you find the links to all of these and add them please?
migration-roadmap/menu.md
Outdated
| - [CSS migration]() | ||
| - [Spectrum 2 preview]() | ||
| - [React]() |
There was a problem hiding this comment.
Can you find the links to these places and add them here?
| <details> | ||
| <summary>Diff: Legacy (CSS main) → Spectrum 2 (CSS spectrum-two)</summary> | ||
|
|
||
| ```diff |
There was a problem hiding this comment.
Can you make sure that Cursor runs the diff on the rendered markup, instead of on the template files? A lot of this isn't relevant to the SWC migration.
There was a problem hiding this comment.
Yep, I think the <details><summary> sections for Legacy and for Spectrum 2 might be identical? Ideally this section would be a diff of what changed between those two sections so we can easily see how the markup changes.
| <details> | ||
| <summary>CSS selectors</summary> | ||
|
|
||
| None found for this component. |
There was a problem hiding this comment.
After the migration, there are 2 action menu selectors. maybe ask cursor to tell you what branches its going to pull this information from before re-running this portion of the prompt.
| ``` | ||
|
|
||
| </details> | ||
|
|
There was a problem hiding this comment.
I like this pattern better too, but I checked the prompt and we're still telling it to omit the section entirely if there's no diff. I would argue that updating the prompt to be more like this avatar example would be better?
FWIW, Avatar, Badge, Help text, Opacity checkerboard, Swatch group, Swatch, and Thumbnail all use this "No significant structural changes" pattern, so it is pretty common in spite of the fact that the prompt doesn't have it.
It might be worth fiddling around in Cursor to see if we can get it to look at the existing docs then update the prompt based on those patterns, but if we go that route, we definitely should have a human read through the prompt changes and make sure they make sense.
| - `selects` - inherit | ||
| - `selects` - single | ||
| - `selects` - multiple |
There was a problem hiding this comment.
Do you want to combine these, sort of like the tooltip placement attribute values from the other PR?
I'm also surprised to see that the description of selects from the API page isn't coming through. Could you add it?
migration-roadmap/menu.md
Outdated
| @sp-menu-submenu-opened=${this.handleDescendentOverlayOpened} | ||
| @sp-menu-submenu-closed=${this.handleDescendentOverlayClosed} | ||
| @slotchange=${this.handleSlotchange} |
There was a problem hiding this comment.
I'm not certain we need the handling called out since they aren't necessary for the DOM.
There was a problem hiding this comment.
Yep, we could honestly just leave it as a <slot></slot>, or a slot with a comment is nice too, like in action group, for example:
<!-- Default slot for sp-menu-item elements -->
<slot></slot>
migration-roadmap/menu.md
Outdated
| <svg class="spectrum-Menu-itemIcon spectrum-Menu-itemIcon--workflowIcon">...</svg> | ||
|
|
||
| <!-- Label: --> | ||
| <span class="spectrum-Menu-itemLabel|spectrum-Menu-sectionHeading spectrum-Switch-label? spectrum-Menu-itemLabel--truncate?"> |
There was a problem hiding this comment.
I almost missed this hidden menu heading class. Do you think it should be separated out at all? I think the thing that tripped me up was that it was under the "Label" comment. Maybe the comment could be updated to include "heading" somehow?
I don't think this is a blocker- it could have just been me!
migration-roadmap/menu.md
Outdated
|
|
||
| | CSS selector | Attribute or slot | Status | | ||
| |-------------|------------------|--------| | ||
| | `.spectrum-Menu` | N/A (root element) | Implemented | |
There was a problem hiding this comment.
We may have to update this table once the selectors have been corrected.
migration-roadmap/menu.md
Outdated
| **Main branch approach:** | ||
| - Uses a `iconWithScale()` helper function that maps size to specific icon variants with scale numbers | ||
| - Example: size "s" → "ArrowLeft200", size "m" → "ArrowLeft300", size "l" → "ArrowLeft400" | ||
| - Applied to chevron icons and back button icons | ||
|
|
||
| **Spectrum 2 approach:** | ||
| - Simplified icon naming - removes the helper function for most icons | ||
| - Uses direct icon names: "ChevronRight", "Checkmark" | ||
| - For back button icons specifically, uses inline size mapping: "ArrowRight" + size suffix ("100", "400", etc.) | ||
| - The size mapping is more explicit and simplified | ||
|
|
||
| **Impact:** This change makes icon usage more consistent and removes the need for a helper function to translate between size values and icon scale numbers. The Spectrum 2 approach is more straightforward and easier to maintain. | ||
|
|
||
| **Additional changes:** | ||
| - Removed theme CSS imports (`../themes/spectrum.css` and `../themes/express.css`) in spectrum-two branch | ||
| - The template structure and DOM markup remain identical between branches |
There was a problem hiding this comment.
Can you update the formatting of this CSS Spectrum 2 changes section to match some of the other components? I was expecting to see some of the new menu stuff (like the thumbnail and external link) to be listed here, but I think since Cursor might have populated this with S1 stuff, it's all missing.
and that would be bad for an SWC migration 😆🤭
|
|
||
| ### CSS | ||
|
|
||
| Action Menu does not have its own CSS component. It is a composite pattern that combines Action Button, Popover, and Menu components. |
There was a problem hiding this comment.
Action menu got migrated in the spectrum-two branch in October, I think? That migration does add a really minimal CSS file that we'll probably want to mention.
| ``` | ||
|
|
||
| </details> | ||
|
|
There was a problem hiding this comment.
I like this pattern better too, but I checked the prompt and we're still telling it to omit the section entirely if there's no diff. I would argue that updating the prompt to be more like this avatar example would be better?
FWIW, Avatar, Badge, Help text, Opacity checkerboard, Swatch group, Swatch, and Thumbnail all use this "No significant structural changes" pattern, so it is pretty common in spite of the fact that the prompt doesn't have it.
It might be worth fiddling around in Cursor to see if we can get it to look at the existing docs then update the prompt based on those patterns, but if we go that route, we definitely should have a human read through the prompt changes and make sure they make sense.
migration-roadmap/menu.md
Outdated
| @sp-menu-submenu-opened=${this.handleDescendentOverlayOpened} | ||
| @sp-menu-submenu-closed=${this.handleDescendentOverlayClosed} | ||
| @slotchange=${this.handleSlotchange} |
There was a problem hiding this comment.
Yep, we could honestly just leave it as a <slot></slot>, or a slot with a comment is nice too, like in action group, for example:
<!-- Default slot for sp-menu-item elements -->
<slot></slot>
migration-roadmap/menu.md
Outdated
| <!-- For icons: --> | ||
| <svg class="spectrum-Menu-itemIcon spectrum-Menu-itemIcon--workflowIcon">...</svg> |
There was a problem hiding this comment.
Spectrum 2 also supports thumbnails before the description, we'll want to include that in the markup here too.
migration-roadmap/menu.md
Outdated
| </span> | ||
| </label> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
We'll also want to include the linkout feature added in S2.
migration-roadmap/menu.md
Outdated
| + }[size] || "100"), | ||
| ``` | ||
|
|
||
| **Key change**: Icon sizing approach was simplified in Spectrum 2. The `iconWithScale()` helper function was removed from the main branch and replaced with simpler inline logic or direct icon names. The back button icon now uses a more explicit size mapping for the ArrowRight icon. |
There was a problem hiding this comment.
iconWithScale appears to exist on both main and spectrum-two, not sure where it got this 🤔
| <details> | ||
| <summary>Diff: Legacy (CSS main) → Spectrum 2 (CSS spectrum-two)</summary> | ||
|
|
||
| ```diff |
There was a problem hiding this comment.
Yep, I think the <details><summary> sections for Legacy and for Spectrum 2 might be identical? Ideally this section would be a diff of what changed between those two sections so we can easily see how the markup changes.
There was a problem hiding this comment.
We'll also have to have migration documentation for menu item, menu divider, and menu group... right now the docs sort of hint at some of those, but I think a big thing that would be really helpful to know is that CSS includes all of those in its menu package whereas SWC breaks them up into separate components.
When we tackle menu for 2nd-gen, I'm not sure if we would try to tackle all of its components (MenuItem, MenuGroup, MenuDivider) all at once but it would be helpful to have a separate section or markdown file that compares each of these. It wouldn't be a component-to-component comparison, because CSS doesn't have all of those components, but we could compare the SWC MenuItem with the parts of the Spectrum-CSS package that relate to menu item.
| - `value` - Value of the selected item(s) | ||
| - `value-separator` - Separator for multiple values (default: ",") | ||
| - `size` (inherited from SizedMixin) | ||
|
|
There was a problem hiding this comment.
We'll want to include attributes and properties from sp-menu-item and other sp-menu components that will map to the CSS selectors that were listed above. I like the idea of putting them in a separate section or even their own file, either one would work. Left a comment with more detail on this above!
7e7651f to
8b8fe5c
Compare
…m-web-components into 2nd-gen-component-analysis
…omponent-analysis-docs

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 components: Menu, Action menu
Motivation and context
Related issue(s)
SWC-1222
Screenshots (if appropriate)
N/A - Documentation only
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
Documentation Quality
Cross-Reference Accuracy
metadata.jsonfilesDevice review