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(theme): add Alert component and theme #10380

Merged
merged 9 commits into from
Jul 21, 2023
Merged

feat(theme): add Alert component and theme #10380

merged 9 commits into from
Jul 21, 2023

Conversation

TylerAPfledderer
Copy link
Contributor

Description

This is part of the Implementation of the DS v1

Adds the Alert component and theming per the new DS

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jun 5, 2023 •
edited
Loading

✅ ethereum-org-website-dev deploy preview ready

@nloureiro
Copy link
Contributor

@TylerAPfledderer the hover effect on the close button for the alerts needs to be the same as the tags, my bad, I did not add this detail to the Figma file. sorry about that

@pettinarip pettinarip mentioned this pull request Jun 15, 2023
Open
pettinarip
pettinarip requested changes Jul 21, 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.

Nice job @TylerAPfledderer

Based on @nloureiro's comment, we need to apply the same styles on the close button as in the Tag component. https://dev--63b7ea99632763723c7f4d6b.chromatic.com/?path=/story/molecules-display-content-tags--style-variants-basic

FYI and context, we also have this other issue #10266 that is going to impact both components styles.

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip right. Would it be fine to handle the close button as a separate PR as before, but one PR to cover both the tag and the alert?

@pettinarip
Copy link
Member

@pettinarip right. Would it be fine to handle the close button as a separate PR as before, but one PR to cover both the tag and the alert?

Make sense. Will approve it then 👍🏼

@pettinarip pettinarip merged commit cf79bc0 into ethereum:dev Jul 21, 2023
@TylerAPfledderer TylerAPfledderer deleted the feat/alert-new-ds-theme branch July 21, 2023 18:04
This was referenced Jul 26, 2023
Merged
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