Skip to content

refactor(avatar)!: use spectrum tokens#1565

Merged
pfulton merged 17 commits intomainfrom
yosevu/css-179-avatar
Feb 6, 2023
Merged

refactor(avatar)!: use spectrum tokens#1565
pfulton merged 17 commits intomainfrom
yosevu/css-179-avatar

Conversation

@yosevu
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread components/avatar/index.css Outdated
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
Copy Markdown
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.

@yosevu yosevu force-pushed the yosevu/css-179-avatar branch from fe0e3e1 to 272987f Compare December 22, 2022 23:33
@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
Copy Markdown
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
Copy Markdown
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.

Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css
Comment thread components/avatar/index.css Outdated
@pfulton
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css Outdated
Comment thread components/avatar/index.css Outdated
@pfulton
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

pfulton commented Jan 11, 2023

Thanks @bernhard-adobe!!

@pfulton pfulton force-pushed the yosevu/css-179-avatar branch from 08f0887 to 94bf397 Compare January 11, 2023 17:49
@pfulton
Copy link
Copy Markdown
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

Comment thread components/avatar/index.css Outdated
}
}

.spectrum-Avatar:focus-within:not(.is-disabled) .spectrum-Avatar-link {
Copy link
Copy Markdown
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
Copy Markdown
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.

@pfulton pfulton force-pushed the yosevu/css-179-avatar branch from 94bf397 to 23115bd Compare February 3, 2023 18:37
@github-actions github-actions bot temporarily deployed to pull request February 3, 2023 18:45 Inactive
Copy link
Copy Markdown
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!

@pfulton pfulton force-pushed the yosevu/css-179-avatar branch from ac4f363 to a66e376 Compare February 6, 2023 14:31
@github-actions github-actions bot temporarily deployed to pull request February 6, 2023 14:36 Inactive
@pfulton pfulton force-pushed the yosevu/css-179-avatar branch from a66e376 to 883c45d Compare February 6, 2023 14:44
@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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants