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

refactor(avatar)!: use spectrum tokens #1565

Merged
merged 17 commits into from
Feb 6, 2023
Merged

refactor(avatar)!: use spectrum tokens #1565

merged 17 commits into from
Feb 6, 2023

Conversation

yosevu
Copy link
Contributor

@yosevu yosevu commented Dec 15, 2022

Description

  • Refactor avatar to use spectrum tokens.
  • Add .spectrum-Avatar-link and focus indicator styles for default variant
  • Add No Link variant

How and where has this been tested?

Tested in Chrome
Tested in WHCM

Screenshots

Screenshot 2022-12-22 at 9 54 54 AM

Screenshot 2022-12-22 at 9 54 27 AM

To-do list

  • I have tested these changes in Windows High Contrast mode.
  • This pull request is ready to merge.

@yosevu yosevu force-pushed the yosevu/css-179-avatar branch 4 times, most recently from efa7a14 to 834e542 Compare December 15, 2022 14:01
@yosevu yosevu changed the title refactor(avatar!): use spectrum tokens refactor(avatar)!: use spectrum tokens Dec 22, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Dec 22, 2022

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

@github-actions github-actions bot temporarily deployed to pull request December 22, 2022 08:43 Inactive
Copy link
Contributor

@bernhard-adobe bernhard-adobe left a comment

Choose a reason for hiding this comment

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

Approving here with a minor question for PJ and Jess in terms of content strategy:

https://adobedesign.slack.com/archives/G019JTYMT6H/p1671723449855909

Screen Shot 2022-12-22 at 16 38 13

box-sizing: border-box;
border-width: var(--spectrum-avatar-border-size);
top: calc(
var(--mod-avatar-focus-indicator-gap, var(--spectrum-avatar-focus-indicator-gap)) *
Copy link
Contributor

Choose a reason for hiding this comment

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

nit. I would place the -2 for both cases on a single line.

@github-actions github-actions bot temporarily deployed to pull request December 22, 2022 23:40 Inactive
@github-actions github-actions bot temporarily deployed to pull request January 3, 2023 21:51 Inactive
@pfulton
Copy link
Collaborator

pfulton commented Jan 3, 2023

Approving here with a minor question for PJ and Jess in terms of content strategy

I don't have a problem with this, as we seem to use his photo in other examples in other components. I can double-check with design folks for the ultimate answer, though.

I pushed up a few more commits here to correct a few things:

  • Here, I think Yosevu might have just accidentally kept the anchor links in the "no link" examples.
  • Here, I bumped the version number for the tokens package, and also included the default selector stuff we've been including in the express.css and spectrum.css files.
  • Here, I restored these modified and removed vars and expressvars files. I don't think these should have been modified or deleted. I'm wondering if Yosevu was having some trouble with style conflicts while he was working on this, but when I restored/reset these files and ran things again, everything looked ok?

Copy link
Contributor

@lunarfusion lunarfusion left a comment

Choose a reason for hiding this comment

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

Mostly suggestions, but I think the focus indicator position does need a change.

components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
@pfulton
Copy link
Collaborator

pfulton commented Jan 4, 2023

Mostly suggestions, but I think the focus indicator position does need a change.

Thanks! I fixed up those things @lunarfusion!

@pfulton pfulton removed their request for review January 4, 2023 16:16
@github-actions github-actions bot temporarily deployed to pull request January 4, 2023 16:18 Inactive
@github-actions github-actions bot temporarily deployed to pull request January 4, 2023 19:52 Inactive
@lunarfusion lunarfusion self-requested a review January 5, 2023 20:08
Copy link
Contributor

@lunarfusion lunarfusion left a comment

Choose a reason for hiding this comment

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

Changes to calc format and to WHCM.

components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
components/avatar/index.css Outdated Show resolved Hide resolved
@pfulton
Copy link
Collaborator

pfulton commented Jan 10, 2023

@lunarfusion - pushed up some changes to address your most recent feedback. This includes the updates to WHCM and the reinstatement of :focus-within. Thanks!

@github-actions github-actions bot temporarily deployed to pull request January 10, 2023 22:30 Inactive
Copy link
Contributor

@lunarfusion lunarfusion left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🚀

@bernhard-adobe
Copy link
Contributor

@pfulton @lunarfusion I am adding the new avatar here in a moment. Maybe please don't merge or release yet.

@bernhard-adobe
Copy link
Contributor

@github-actions github-actions bot temporarily deployed to pull request January 11, 2023 16:35 Inactive
@pfulton
Copy link
Collaborator

pfulton commented Jan 11, 2023

Thanks @bernhard-adobe!!

@pfulton
Copy link
Collaborator

pfulton commented Jan 11, 2023

Beta released: @spectrum-css/avatar@6.0.0-beta.0

Tracking the integration here: adobe/spectrum-web-components#2844

@pfulton pfulton added released-beta Indicates a beta release has been rolled for this PR pending-swc-validation Being reviewed in/by SWC; required for breaking changes labels Jan 11, 2023
}
}

.spectrum-Avatar:focus-within:not(.is-disabled) .spectrum-Avatar-link {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be: .spectrum-Avatar:not(.is-disabled) .spectrum-Avatar-link:focus-visible and have the same functional styles, right? The main difference that we've not discussed for :focus-within is that is doesn't match the keyboard heuristics of :focus-visible...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, that'll work.

And yes, we definitely have a discussion about how best to serve :focus-* to y'all going forward.

@github-actions github-actions bot temporarily deployed to pull request February 3, 2023 18:45 Inactive
Copy link
Contributor

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

Works for SWC! Graduate a stable release and we can get adobe/spectrum-web-components#2844 closed out as well!

@github-actions github-actions bot temporarily deployed to pull request February 6, 2023 14:36 Inactive
@github-actions github-actions bot temporarily deployed to pull request February 6, 2023 14:50 Inactive
@pfulton pfulton merged commit 8db2337 into main Feb 6, 2023
@pfulton pfulton deleted the yosevu/css-179-avatar branch February 6, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending-swc-validation Being reviewed in/by SWC; required for breaking changes released-beta Indicates a beta release has been rolled for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants