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(avatar): migrate s2 avatar #3355

Open
wants to merge 2 commits into
base: spectrum-two
Choose a base branch
from

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Oct 30, 2024

Description

Adds support for new avatar-size tokens (avatar-size-800 to avatar-size-1500). Updates avatar component story to support new sizes. Adds support for avatar-border-color and avatar-border-thickness.

Validation steps

  1. Access the Storybook URL for the PR (or fetch the branch and run it locally)
  2. Navigate to the avatar story
  3. Toggle through the new sizes added to the avatar story, verify they behave as expected and align with the added avatar sizes.

How and where has this been tested?

Locally in Storybook.

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

Screenshot 2024-10-30 at 12 40 27 PM Screenshot 2024-10-30 at 12 40 21 PM

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. ✨

Copy link

changeset-bot bot commented Oct 30, 2024

🦋 Changeset detected

Latest commit: ac9fd79

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

This PR includes changesets to release 3 packages
Name Type
@spectrum-css/avatar Major
@spectrum-css/tag Major
@spectrum-css/taggroup 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

Copy link
Contributor

github-actions bot commented Oct 30, 2024

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

Copy link
Contributor

github-actions bot commented Oct 30, 2024

File metrics

Summary

Total size: 4.29 MB*
Total change (Δ): ⬆ 5.33 KB (0.12%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package Size Δ
avatar 6.56 KB ⬆ 1.54 KB

Details

avatar

File Head Base Δ
index-base.css 6.56 KB 5.02 KB ⬆ 1.54 KB (30.79%)
index-vars.css 6.56 KB 5.02 KB ⬆ 1.54 KB (30.79%)
index.css 6.56 KB 5.02 KB ⬆ 1.54 KB (30.79%)
metadata.json 2.46 KB 1.76 KB ⬆ 0.71 KB (39.45%)
* 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.

@cdransf cdransf marked this pull request as ready for review October 30, 2024 19:42
@cdransf cdransf self-assigned this Oct 30, 2024
@cdransf cdransf added size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Oct 30, 2024
@cdransf cdransf force-pushed the cdransf/spectrum-two-avatar-migration branch from 82c08b5 to f322aaa Compare October 30, 2024 19:49
@cdransf cdransf force-pushed the cdransf/spectrum-two-avatar-migration branch from f322aaa to 3d6fcf2 Compare November 4, 2024 16:16
@cdransf cdransf changed the title cdransf/spectrum two avatar migration feat(avatar): migrate s2 avatar Nov 5, 2024
Copy link
Collaborator

@5t3ph 5t3ph left a comment

Choose a reason for hiding this comment

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

All sizes match the Figma ✨

--spectrum-avatar-border-radius: var(--spectrum-avatar-block-size);
--spectrum-avatar-border-radius: var(--spectrum-avatar-block-size);
--spectrum-avatar-border-thickness: var(--spectrum-avatar-border-width);
--spectrum-avatar-border-color: rgb(var(--spectrum-gray-25-rgb));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I actually went back and tried this with the token from the design, but changed the custom property name a touch, and I think everything is working ok now.

--spectrum-avatar-border-color-default: var(--spectrum-avatar-border-color);

I wonder if we had some caching issues? Because, it was definitely coming up undefined right? Maybe tweaking the custom property and adding that -default helped?

Copy link
Member Author

Choose a reason for hiding this comment

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

So strange! I see it working now too. Just updated the PR. ✨

@cdransf cdransf force-pushed the cdransf/spectrum-two-avatar-migration branch from 3d6fcf2 to bfdab94 Compare November 6, 2024 17:02
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.

This is looking really good to me. Now that we added the -default to the avatar border property, I think we just need to update the name in .spectrum-Avatar too.

I also had one thought about adding the new sizes to the docs page (eventually).

border-style: solid;
border-radius: var(--mod-avatar-border-radius, var(--spectrum-avatar-border-radius));
border-width: var(--spectrum-avatar-border-thickness);
border-color: var(--spectrum-avatar-border-color);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should change this to the --spectrum-avatar-border-color-default!

@@ -15,7 +15,7 @@ export default {
type: { summary: "string" },
category: "Component",
},
options: ["50", "75", "100", "200", "300", "400", "500", "600", "700"],
options: ["50", "75", "100", "200", "300", "400", "500", "600", "700", "800", "900", "1000", "1100", "1200", "1300", "1400", "1500"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

ooo! Would you please make a card to add these new sizes to the table we have on the docs page on main? I was going to ask you to add them, and then realized the docs page on this branch is totally different than what's on main! We should make sure we reference the new sizes on that page eventually, once the big merge happens.

Screenshot 2024-11-06 at 11 59 32 AM

@cdransf cdransf force-pushed the cdransf/spectrum-two-avatar-migration branch from bfdab94 to ac9fd79 Compare November 6, 2024 17:30
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.

Looks great! All of the sizes are working, I see all of the new size tokens, border width and border color tokens! Glad we got the border color thing figured out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review size-3 M ~18-30hrs; moderate effort or complexity, several work days needed. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants