chore: spectrum-two fixes by marissahuysentruyt · Pull Request #3514 · adobe/spectrum-css

marissahuysentruyt · 2025-01-21T17:49:20Z

Description

This PR reimplements some fixes for the spectrum-two branch after a rebase.

Adds a missing variant = "neutral" default arg to the tooltip template. This should alleviate the errors on the docs page.

Before 🚫

After ✅

An additional fix on the steplist docs page occurred since the WithTooltip story calls for the tooltip component. 🥳

Before 🚫

After ✅

The calls to window.isChromatic have been removed. It also corrects the arg names for picker, in order for them to be properly passed to the popover used in the template and render the content. This should alleviate all errors and most of the missing content on the docs page. (additional styling and content changes will need to be addressed in a separate PR).

Before 🚫

After ✅

An additional fix on the form docs page occurred since the examples we use contain the picker component. 🥳

Before 🚫

After ✅

An additional fix on the tabs docs page occurred since the Overflow story calls for the picker component. 🥳

Before 🚫

After ✅

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Pull down the branch to run locally or use the deploy preview.
Visit the picker docs page.
Verify no errors referencing the variant is not defined appear in place of the WithTooltip story canvas. Each story step in the steplist should have a rendered tooltip with step number content.
Visit the form docs page.
Similarly to the picker docs, now errors should occur regarding window.isChromatic(). All form components should render with a closed picker component that displays the placeholder text.
Visit the tooltip docs page.
Verify there are no errors referencing variant is not defined. All tooltips should now display as expected.
Visit the tabs docs page.
Similarly to the picker docs, no errors should occur regarding window.isChromatic(). The overflow tabs components should render with a closed picker component that displays the first tab item's text & chevron.
Visit the steplist docs page.
Verify

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
If my change impacts other components, I have tested to make sure they don't break.
✨ This pull request is ready to merge. ✨

changeset-bot · 2025-01-21T17:49:24Z

⚠️ No Changeset found

Latest commit: 392cb52

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2025-01-21T17:53:53Z

File metrics

Summary

Total size: 2.23 MB^*

🎉 No changes detected in any packages

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

github-actions · 2025-01-21T22:21:53Z

🚀 Deployed on https://pr-3514--spectrum-css.netlify.app

marissahuysentruyt · 2025-01-22T14:47:47Z

 		isHovered,
 		isActive,
-		content: { table: { disable: true } },
+		popoverContent: { table: { disable: true } },


During the picker docs-to-storybook migration, we tried to clarify this picker control by calling it popoverContent. This is the content that gets passed to the popover component in the template.

Popover has its own content arg, so that's why you'll see the content: popoverContent assignment.

Separately, there might be more that we can pull from the docs migration PR listed above. It looks like some of those additional controls/args (like currentValue, isHovered, showWorkflowIcon, and maybe some others) are missing currently. That could probably be figured out in a separate PR.

marissahuysentruyt · 2025-01-22T14:49:46Z

 	rootClass = "spectrum-Tooltip",
 	label,
 	placement,
+	variant = "neutral",


Looks like the informative & negative tooltips got added back to Figma 11/18/24 (approximately). We'll have to create a card to implement the CSS styles and stories (for the docs page & controls)

variant: { name: "Variant", type: { name: "string" }, table: { type: { summary: "string" }, category: "Component", }, options: ["neutral", "info", "negative"], control: "inline-radio",

tooltip migration PR: https://github.com/adobe/spectrum-css/pull/2743/files#diff-14d4b487bc2de8da9dccaef67bce6cccc92cbce28e2ac16912884edbce436cbc

🙀 Good catch! Who knew those variants would come back again?

marissahuysentruyt · 2025-01-22T16:06:33Z

 			type="button"
-			@click=${() => {
-				if (window.isChromatic()) return;
+			@click=${function() {
 				updateArgs({ isOpen: !isOpen });
 			}}


On main, this line is @click=${onclick}, and the onclick function was defined in the Popover call. This template is pretty different from main, so I plopped the function here for now.

Any thoughts or reservations on this?

Just a nit, but should we keep it formatted as an arrow function? ✨

I'm not sure I know enough about why this part of the code is different between the branches/how Chromatic normally handles Picker, but it does work as is, and would work as an arrow function as well 🤷‍♀️

cdransf

Ran through the validation steps and the only error I encountered were due to missing theme css files (which is out of scope and — I believe — fixed on spectrum-two).

cdransf · 2025-01-22T16:27:31Z

 			type="button"
-			@click=${() => {
-				if (window.isChromatic()) return;
+			@click=${function() {
 				updateArgs({ isOpen: !isOpen });
 			}}


Just a nit, but should we keep it formatted as an arrow function? ✨

rise-erpelding

This works and fixes many of the other issues I was noticing in other components too!

This does a nice job preserving the changes from the picker migration in #2697 but keeps what we need to make things work! 🎉

rise-erpelding · 2025-01-22T17:11:59Z

 			type="button"
-			@click=${() => {
-				if (window.isChromatic()) return;
+			@click=${function() {
 				updateArgs({ isOpen: !isOpen });
 			}}


I'm not sure I know enough about why this part of the code is different between the branches/how Chromatic normally handles Picker, but it does work as is, and would work as an arrow function as well 🤷‍♀️

rise-erpelding · 2025-01-22T17:17:02Z

 	rootClass = "spectrum-Tooltip",
 	label,
 	placement,
+	variant = "neutral",


🙀 Good catch! Who knew those variants would come back again?

- Tooltip was missing the `variant = "neutral"` arg, so adding that back in renders the component

- adds missing placeholder arg to template - corrects content arg to popoverContent

* docs(tooltip): add missing args to template - Tooltip was missing the `variant = "neutral"` arg, so adding that back in renders the component * chore(picker): add/correct arg naming - adds missing placeholder arg to template - corrects content arg to popoverContent * chore(picker): remove window.isChromatic calls and click event

marissahuysentruyt changed the title ~~Marissahuysentruyt/spectrum two context fixes~~ chore: spectrum-two fixes Jan 21, 2025

castastrophe force-pushed the marissahuysentruyt/spectrum-two-context-fixes branch from 7099b51 to 7ec37a6 Compare January 21, 2025 19:52

marissahuysentruyt force-pushed the marissahuysentruyt/spectrum-two-context-fixes branch from 7ec37a6 to db363d5 Compare January 21, 2025 22:20

marissahuysentruyt commented Jan 22, 2025

View reviewed changes

marissahuysentruyt mentioned this pull request Jan 22, 2025

chore(picker): update/fix picker args #3506

Closed

9 tasks

marissahuysentruyt force-pushed the marissahuysentruyt/spectrum-two-context-fixes branch 2 times, most recently from 8f25da7 to c038203 Compare January 22, 2025 16:05

marissahuysentruyt commented Jan 22, 2025

View reviewed changes

marissahuysentruyt self-assigned this Jan 22, 2025

marissahuysentruyt added S2 Spectrum 2 bug Results from a bug in the CSS implementation size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. labels Jan 22, 2025

marissahuysentruyt marked this pull request as ready for review January 22, 2025 16:17

marissahuysentruyt added the ready-for-review label Jan 22, 2025

marissahuysentruyt requested review from 5t3ph, aramos-adobe, castastrophe, cdransf, jawinn and rise-erpelding January 22, 2025 16:21

marissahuysentruyt mentioned this pull request Jan 22, 2025

docs(progressbar, tooltip): reimplement default args in template #3485

Closed

10 tasks

cdransf approved these changes Jan 22, 2025

View reviewed changes

rise-erpelding approved these changes Jan 22, 2025

View reviewed changes

castastrophe force-pushed the spectrum-two branch from 7e783b6 to e3585cd Compare January 22, 2025 17:44

marissahuysentruyt added 2 commits January 22, 2025 13:22

docs(tooltip): add missing args to template

72dd631

- Tooltip was missing the `variant = "neutral"` arg, so adding that back in renders the component

chore(picker): add/correct arg naming

fee9e00

- adds missing placeholder arg to template - corrects content arg to popoverContent

chore(picker): remove window.isChromatic calls and click event

392cb52

marissahuysentruyt force-pushed the marissahuysentruyt/spectrum-two-context-fixes branch from c038203 to 392cb52 Compare January 22, 2025 18:23

marissahuysentruyt merged commit f360c68 into spectrum-two Jan 22, 2025

marissahuysentruyt deleted the marissahuysentruyt/spectrum-two-context-fixes branch January 22, 2025 18:40

castastrophe mentioned this pull request Apr 15, 2025

feat(tokens,card): update to @adobe/spectrum-tokens@13.3.0 #3669

Merged

12 tasks

This was referenced May 16, 2025

chore(swatch,menu,swatchgroup): fix reverted metadata assets #3752

Merged

chore: merge latest updates from main #3777

Merged

castastrophe mentioned this pull request Jul 25, 2025

ci: chromatic runs for S2 [CSS-1233] #4070

Merged

3 tasks

castastrophe mentioned this pull request Aug 6, 2025

fix: minor linting updates #4113

Merged

11 tasks

This was referenced Aug 27, 2025

chore(popover): wrapper placement in story #4173

Closed

feat(storybook): chromatic a11y testing #4180

Closed

chore: updating lint-staged file inheritance order #4193

Closed

castastrophe mentioned this pull request Oct 22, 2025

feat(badge): remove modifiers from the API [SWC-1264] #4283

Merged

14 tasks

Conversation

marissahuysentruyt commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

Uh oh!

changeset-bot Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

github-actions Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

File metrics

Summary

Uh oh!

github-actions Bot commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marissahuysentruyt Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdransf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rise-erpelding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

marissahuysentruyt commented Jan 21, 2025 •

edited

Loading

changeset-bot Bot commented Jan 21, 2025 •

edited

Loading

github-actions Bot commented Jan 21, 2025 •

edited

Loading

github-actions Bot commented Jan 21, 2025 •

edited

Loading

marissahuysentruyt Jan 22, 2025 •

edited

Loading