-
Notifications
You must be signed in to change notification settings - Fork 294
feat(components-library) Social Links Drawer #3090
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
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is pretty solid! However, the styling is off -- it doesn't match the link styling elsewhere in the design system, and the cards are huge at smaller screen sizes, e.g. here's a smaller browser window:

And here's a mobile screen:

I suggest we have @partyparrotgreg take a quick pass at this and match his suggestion. I think that card-style links probably aren't the right presentation here.
|
||
.communityCard { | ||
aspect-ratio: 1; | ||
min-height: 160px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please stick to theme.spacing
for spacing units
&:hover { | ||
transform: translateY(-2px); | ||
box-shadow: 0 8px 25px rgb(0 0 0 / 15%); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hover styling appears nowhere else in the design system; it's inconsistent and we should make this match everything else
svg { | ||
width: 100%; | ||
height: 100%; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This styling should be unneeded, since you're using phosphor icons the default width and height are set to 1em
. This means that the right way to style them is to set font-size: theme.spacing(12)
on .communityIcon
instead of setting width
and height
and having the styling here.
I will wait for @partyparrotgreg here then. |
Summary
Rationale
How has this been tested?