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.

Sign up for GitHub

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

Jump to bottom

feat(Avatar): create component, theme, and stories #9868

Merged
merged 16 commits into from
Jul 19, 2023
Merged

feat(Avatar): create component, theme, and stories #9868

merged 16 commits into from
Jul 19, 2023

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Apr 3, 2023 •
edited
Loading

Description

This PR creates the Avatar component per the new DS

  • Builds the component and Chakra theme
  • Adds a set of stories per the DS requirements
    • Avatars are always rendered as links
    • Story for Single Avatars
    • Story for a Group of Avatars
    • Story for single avatars with text (in the form of using the LinkBox/LinkOverlay component pair)

Related Issue

This is a part of the implementation of the DS v1 #9548

@gatsby-cloud
Copy link

gatsby-cloud bot commented Apr 3, 2023 •
edited
Loading

✅ ethereum-org-website-dev deploy preview ready

@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review April 8, 2023 01:39
TylerAPfledderer
TylerAPfledderer commented Apr 8, 2023
View reviewed changes
.storybook/main.js Outdated Show resolved Hide resolved
TylerAPfledderer
TylerAPfledderer commented Apr 8, 2023
View reviewed changes
.storybook/main.js Outdated Show resolved Hide resolved
@pettinarip pettinarip mentioned this pull request Apr 14, 2023
Open
@nloureiro
Copy link
Contributor

@TylerAPfledderer and @pettinarip Made a few changes to the DS Figma file and commented on the changes there

https://www.figma.com/file/NrNxGjBL0Yl1PrNrOT8G2B/ethereum.org-Design-System?type=design&node-id=2402%3A45505&t=ggIw3B0PhQylUIH8-1

Let me know if you need more details?

pettinarip
pettinarip reviewed Jun 28, 2023
View reviewed changes
src/@chakra-ui/gatsby-plugin/components/Avatar.ts Outdated
[$border.variable]: "transparent",
borderWidth: "1px",
"&:hover, [data-peer]:hover ~ &": {
boxShadow: "buttonHover",
Copy link
Member

Choose a reason for hiding this comment

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

Can't remember where we ended up with this but talking async with @nloureiro, we said that it would be nice if we can use a more dynamic shadow size for the Avatar.

// we tested with 0.15em and its fine
boxShadow="0.15em 0.15em 0 color"

but I think this will only apply for the Avatar, the other components should keep using the primary shadow.

Let me know what do you think on this implementation @TylerAPfledderer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip I've pushed this change for you to view.

Though it looks very minor. I'm seeing clipping which does not occur if pixel values are applied.
image

Not sure what is causing this on my end.

pettinarip
pettinarip reviewed Jun 28, 2023
View reviewed changes
src/@chakra-ui/gatsby-plugin/components/Avatar.ts
[$mlBySize.variable]: "space.-1",
},
excessLabel: {
fontSize: "0.563rem",
Copy link
Member

Choose a reason for hiding this comment

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

Curious about this value. How did you get it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the DS for the xs size avatar, the excess label is showing a font size of 9px which is this in rem.

This value is not a part of the default fontSize tokens. 2xs in the default theme is 0.625rem, and 3xs is 0.45rem.

src/@chakra-ui/gatsby-plugin/semanticTokens.ts Outdated
_light: "4px 4px 0 var(--eth-colors-blue-100)",
_dark: "4px 4px 0 #352313",
},
},
Copy link
Member

Choose a reason for hiding this comment

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

Nice, is the idea to delete the shadows file at some point? or should we still keep using that file to separate a bit the tokens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip this is here because a semantic color is not being declared in the DS for this style of shadow, so a semantic has to be made for each color based on color mode.

Copy link
Member

Choose a reason for hiding this comment

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

We need to use primary.lowContrast here but our orange.800 is outdated.

We need to update orange.800 to #352313 and then we can use primary.lowContrast

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the latest updates from the base branch, this has taken care of itself. Still have to push a commit to remove buttonHover from semanticTokens.

@nloureiro
Copy link
Contributor

The spacing and size of the shadow still is a blocker for me, I might be neat picking but would be cool if we could adjust this

Screen Shot 2023-06-29 09 23 18 AM

@pettinarip
Copy link
Member

The spacing and size of the shadow still is a blocker for me, I might be neat picking but would be cool if we could adjust this

What do you exactly mean with the spacing? the current spacing in the code is 0.25rem = 4px. On the other side, we are going to implement a dynamic shadow using 0.15em but the spacing is going to be the same always.

@nloureiro
Copy link
Contributor

The spacing and size of the shadow still is a blocker for me, I might be neat picking but would be cool if we could adjust this

What do you exactly mean with the spacing? the current spacing in the code is 0.25rem = 4px. On the other side, we are going to implement a dynamic shadow using 0.15em but the spacing is going to be the same always.

I think we need to bump the spacing to 8px, between the avatar and the username

Screen Shot 2023-06-29 06 42 18 PM

pettinarip
pettinarip requested changes Jun 29, 2023
View reviewed changes
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

Ok, in summary, I think what is left in this PR is:

  • dynamic shadow using em
  • fix shadow color using primary.lowContrast and updating the orange.800 color
  • fix the spacing from 4px to 8px

@TylerAPfledderer
Copy link
Contributor Author

Ok, in summary, I think what is left in this PR is:

* dynamic shadow using `em`

* fix shadow color using `primary.lowContrast` and updating the `orange.800` color

* fix the spacing from 4px to 8px

All items have been addressed.

As to the spacing between avatar and link text, the DS shows padding around the text container, which contributes to the larger gap.

So there is a 4px gap between the avatar and text container, plus 4px padding in the text container.

pettinarip
pettinarip approved these changes Jul 19, 2023
View reviewed changes
Copy link
Member

@pettinarip pettinarip left a comment

Choose a reason for hiding this comment

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

I've a question about the font sizes used in the avatar text but not a blocker. Good job!

@TylerAPfledderer @pettinarip
Update src/components/Avatar/index.tsx
a76ae3e 
Co-authored-by: Pablo Pettinari <pettinarip@gmail.com>
@pettinarip pettinarip merged commit d29b6a8 into ethereum:dev Jul 19, 2023
@corwintines corwintines mentioned this pull request Jul 19, 2023
Merged
@TylerAPfledderer TylerAPfledderer deleted the feat/new-avatar-theme branch July 19, 2023 21:54
@corwintines corwintines mentioned this pull request Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pettinarip pettinarip pettinarip approved these changes

@samajammin samajammin Awaiting requested review from samajammin

@wackerow wackerow Awaiting requested review from wackerow wackerow is a code owner

@corwintines corwintines Awaiting requested review from corwintines corwintines is a code owner

@minimalsm minimalsm Awaiting requested review from minimalsm minimalsm is a code owner

@jmcook1186 jmcook1186 Awaiting requested review from jmcook1186

@nhsz nhsz Awaiting requested review from nhsz

Assignees

@TylerAPfledderer TylerAPfledderer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TylerAPfledderer @nloureiro @pettinarip