Skip to content
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

feat(helptext): S2 migration #3628

Merged
merged 20 commits into from
Apr 1, 2025

Conversation

aramos-adobe
Copy link
Contributor

Help text S2 migration

The helptext S2 component has some updated spacing and typography tokens. The error message validation icon has also been updated.

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

  • Open the storybook for the helptext component
  • Check the icon look correct

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. 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.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@aramos-adobe aramos-adobe self-assigned this Mar 17, 2025
@aramos-adobe aramos-adobe added size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day. run_vrt For use on PRs looking to kick off VRT ready-for-review S2 Spectrum 2 labels Mar 17, 2025
@aramos-adobe aramos-adobe force-pushed the aramos-adobe/css709-helptext-s2-migration branch from 3b33028 to 3548b58 Compare March 17, 2025 21:11
Copy link

changeset-bot bot commented Mar 17, 2025

🦋 Changeset detected

Latest commit: f7354ee

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/helptext Major

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

@aramos-adobe aramos-adobe added skip_vrt Add to a PR to skip running VRT (but still pass the action) and removed run_vrt For use on PRs looking to kick off VRT labels Mar 17, 2025
Copy link
Contributor

github-actions bot commented Mar 20, 2025

File metrics

Summary

Total size: 1.37 MB*

Package Size Minified Gzipped
helptext 6.93 KB 6.64 KB 1.12 KB

helptext

Filename Head Minified Gzipped Compared to base
index.css 6.93 KB 6.64 KB 1.12 KB 🟢 ⬇ 0.18 KB
metadata.json 3.77 KB - - 🔴 ⬆ 0.48 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

Copy link
Contributor

github-actions bot commented Mar 20, 2025

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

@castastrophe castastrophe force-pushed the aramos-adobe/css709-helptext-s2-migration branch 2 times, most recently from 7e53fa1 to 884a119 Compare March 21, 2025 14:45
@castastrophe castastrophe added run_vrt For use on PRs looking to kick off VRT and removed skip_vrt Add to a PR to skip running VRT (but still pass the action) run_vrt For use on PRs looking to kick off VRT labels Mar 21, 2025
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is almost there! There are some adjustments needed to the sizing, font, and text color mostly.

I don't think we need to do much to the tests/template/story file, since this is a pretty small migration, but it might be a nice addition to test multiline as well as single line text with the negative icon next to it.

Nice work!

}

.is-disabled {
--spectrum-helptext-content-color-default: var(--spectrum-disabled-content-color);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figma is wrong here, right? Because there is no disabled-content-color-default token?

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's close! The tokens need some cleaning up in a few places for the t-shirt sizes. I didn't want to litter your PR with similar comments over and over, but I think we need to double check the following for each t-shirt size:

font-size-*
component-height-* (I did leave a comment or two regarding these tokens)
text-to-visual-*
top-to-text-* bottom-to-text-*`

For each of our class sizes, the size/number for the tokens should all be the same according to the token specs, and they're a little mixed up right now, that's all.

Thanks for getting this component back up and ready!

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit

stylelint

⚠️ [stylelint] <declaration-block-no-duplicate-custom-properties> reported by reviewdog 🐶
Unexpected duplicate "--spectrum-toast-background-color-default"

--spectrum-toast-background-color-default: var(--spectrum-neutral-subdued-background-color-default);


⚠️ [stylelint] <declaration-block-no-duplicate-custom-properties> reported by reviewdog 🐶
Unexpected duplicate "--spectrum-toast-divider-color"

--spectrum-toast-divider-color: var(--spectrum-transparent-white-400);


⚠️ [stylelint] <declaration-block-no-duplicate-custom-properties> reported by reviewdog 🐶
Unexpected duplicate "--spectrum-well-border-color"

--spectrum-well-border-color: rgba(var(--spectrum-gray-1000-rgb), .05);

@aramos-adobe aramos-adobe force-pushed the aramos-adobe/css709-helptext-s2-migration branch from 1a417e5 to a3bf036 Compare March 26, 2025 17:20
Copy link
Collaborator

@rise-erpelding rise-erpelding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking for me here! I see that the sizing has been adjusted, our fonts look good with the font stack, and otherwise the tokens appear to be used as we'd expect!

Left a few comments with small suggestions, but again, I don't consider those blocking. Nice work!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the top .spectrum-HelpText (L15 and L16), we have:

	--spectrum-helptext-content-color-default: var(--spectrum-neutral-subdued-content-color-default);
	--spectrum-helptext-icon-color-default: var(--spectrum-neutral-subdued-content-color-default);

and then further down (around L70/71), we also have:

	&.spectrum-HelpText--neutral {
		--spectrum-helptext-content-color-default: var(--spectrum-neutral-subdued-content-color-default);
		--spectrum-helptext-icon-color-default: var(--spectrum-neutral-subdued-content-color-default);
	}

Wouldn't it save us a couple of lines (or I guess, a line, really) and essentially behave the same to change that to something like:

        &,
	&.spectrum-HelpText--neutral {
		--spectrum-helptext-content-color-default: var(--spectrum-neutral-subdued-content-color-default);
		--spectrum-helptext-icon-color-default: var(--spectrum-neutral-subdued-content-color-default);
	}

There's not some specific reason I'm missing that we need it up at the top, is there?

Honestly, not a huge deal, just more of a nit.

Noting also this does not appear to have been introduced in the migration.

Comment on lines 146 to 151
forced-color-adjust: none;

.spectrum-HelpText-validationIcon,
.spectrum-HelpText-text {
forced-color-adjust: none;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what forced-color-adjust: none; does is that in WHCM it tells the user agent styles not to make any default adjustments for forced colors mode and to only use the styles provided by our CSS. Given that this is such a simple component (text and icon, basically, with no changes to hover/focus etc. states), we might be able to remove these without any negative effect.

Comment on lines 93 to 94
padding-block-start: var(--mod-helptext-edge-to-workflow-icon, var(--spectrum-helptext-edge-to-workflow-icon));
padding-block-end: var(--mod-helptext-edge-to-workflow-icon, var(--spectrum-helptext-edge-to-workflow-icon));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think I like this better than keeping top and bottom separate variables! Pointing out though that we do lose the ability to modify different spacings for top vs. bottom. Given that design specs often point out when it's really important that consumers be able to modify something, and it's not here, I think using a single variable is ok, but pointing this out anyway in case someone has a stronger opinion about it.

Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing

Comment on lines +5 to +7
### Helptext S2 Migration

The helptext S2 component has some updated spacing and typography tokens. The error message validation icon has also been updated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only because we just talked about detailed changesets with SWC, should we list out any of the new or renamed mods created during this work? We were doing that back in the fall, but I think we got away from it after we had been focused on foundations for so long! Looks like it's probably only 3 new mods:

"--mod-helptext-font-family",
"--mod-helptext-font-style",
"--mod-helptext-font-weight",

and then one renamed:
"--mod-helptext-bottom-to-workflow-icon" >> "--mod-helptext-edge-to-workflow-icon",

You'd know better than me on those! It's up to you, I won't let it block my approval.

@aramos-adobe aramos-adobe merged commit beee947 into spectrum-two Apr 1, 2025
12 checks passed
@aramos-adobe aramos-adobe deleted the aramos-adobe/css709-helptext-s2-migration branch April 1, 2025 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-1 XS ~1-6hrs; nearly trivial, a few hours, could do more than one in a single day.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants