Skip to content

[ShadCN]: Implement ShadCN Avatar to Production#14814

Merged
pettinarip merged 3 commits into
ethereum:devfrom
TylerAPfledderer:feat/shadCN-avatar-implement
Feb 5, 2025
Merged

[ShadCN]: Implement ShadCN Avatar to Production#14814
pettinarip merged 3 commits into
ethereum:devfrom
TylerAPfledderer:feat/shadCN-avatar-implement

Conversation

@TylerAPfledderer
Copy link
Copy Markdown
Contributor

@TylerAPfledderer TylerAPfledderer commented Feb 3, 2025

Description

Applies the newest Avatar component to all current instances: IssuesList.tsx, FileContributors.tsx, and Leaderboard.tsx

Two points of note related to the Design System:

  • It is intentional in Leaderboard.tsx to adhere to the DS, where it is required to supply a value to the href prop. It is believed that it is better to address this requirement in the short term with a comment and using # as the value in the component instance, instead of making the prop an optional type. This should not be noticeable to the user.
  • There are multiple instance where the height and width are set to the token value 10. This value is not being used in any of the size variants in the DS, and therefore explicitly set in the given instances.

Related Issue

Minor impact approaching migrations done for the above affected components in #13946

@netlify
Copy link
Copy Markdown

netlify Bot commented Feb 3, 2025

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 1c14c28
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/67a28dbb52dcb20008172208
😎 Deploy Preview https://deploy-preview-14814--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 49 (🟢 up 1 from production)
Accessibility: 95 (🟢 up 3 from production)
Best Practices: 89 (🔴 down 9 from production)
SEO: 98 (no change from production)
PWA: 59 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Comment thread src/components/FileContributors.tsx
Comment thread src/components/Leaderboard.tsx Outdated
w={10}
display={{ base: "none", xs: "block" }}
// This meets the Design System requirement, despite not being used in this instance
href="#"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we align it with the link for the row?

Suggested change
href="#"
href={hasGitHub ? `${GITHUB_URL}${username}` : "#"}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If not the user gets this ux where the avatar link is upside the linkbox making it a weird experience

02-03-2025-16.11.56.mp4

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I wasn't expecting that! I'll go ahead with your suggestion here.

For future consideration the avatar shouldn't really be hoverable here unless styled based on hover over the rest of the list item, correct?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, correct. For this transition, we will leave it as is, but ideally only the row should be the element that gets the hover effect.

@TylerAPfledderer
Copy link
Copy Markdown
Contributor Author

@pettinarip should be all set with the changes here!

Copy link
Copy Markdown
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.

LGTM! thanks @TylerAPfledderer

@pettinarip pettinarip merged commit 026636e into ethereum:dev Feb 5, 2025
@TylerAPfledderer TylerAPfledderer deleted the feat/shadCN-avatar-implement branch February 5, 2025 10:10
This was referenced Feb 12, 2025
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.

2 participants