Skip to content

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Sep 27, 2024

Description

  • Updates ui/alert with modifications to theming per latest design system
  • Adds AlertEmoji to ui/alert, with default styling matching the old Emoji portion of the InfoBanner component
  • Migrates usage of InfoBanner to use Alert component structure instead.
  • Updates EnvWarningBanner to use ui/alert. A few markdown pages had the manual addition of effectively the same component as the EnvWarningBanner component, so this was removed and replaced with the component (now included in MdComponents).

Related Issue

shadcn migration and theming updates

Update all `<InfoBanner shouldSpaceBetween emoji=":eyes:">{children}</InfoBanner>` instances to use `<Alert className="justify-between"><AlertEmoji text=":eyes:" />{children}</Alert>`
Convert all `<InfoBanner emoji="text">` to `<Alert><AlertEmoji text="text"/><AlertContent>`
Migrate all isWarning to variant="warning"
Migrate all vanilla `<InfoBanner>` instances to ui/alert
Previous `InfoBanner` components with `isWarning` flag initially migrated with `warning` variant, which is yellow, but should be red: switched to `error` variant to match
all instances with mb-8 and title
update all `<InfoBanner mb={8}` instances
update all `<InfoBanner isWarning>` instances
migrate all `<InfoBanner isWarning mb={8}>` instances
migrate all `<InfoBanner shouldCenter emoji=` instances
migrate all `<InfoBanner shouldSpaceBetween emoji="" mt="8">` instances
@netlify
Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 98bbe6d
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/66fd78b41881ed0008a29b8f
😎 Deploy Preview https://deploy-preview-13986--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: 51 (🟢 up 5 from production)
Accessibility: 93 (no change from production)
Best Practices: 89 (🔴 down 9 from production)
SEO: 92 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits translation 🌍 This is related to our Translation Program labels Sep 27, 2024
@wackerow wackerow marked this pull request as draft September 27, 2024 14:16
@wackerow
Copy link
Member Author

Switching to "Ready for review" to get eyes 👀 on this, but leaving as "Work In Progress" for at least a few days while we comb through this... touches hundreds of files, so let's make sure we've done plenty of QA checking on this.

@wackerow wackerow added needs design approval 🧑‍🎨 Approval from a designer is needed before merging needs review 👀 labels Sep 30, 2024
@nloureiro
Copy link
Contributor

Should the alert have some default margins to prevent this from happening?

Screen Shot 2024-10-01 11 28 46 AM

@nloureiro
Copy link
Contributor

nloureiro commented Oct 1, 2024

@nloureiro
Copy link
Contributor

nloureiro commented Oct 1, 2024

center alignment should be left

https://deploy-preview-13986--ethereumorg.netlify.app/en/guides/how-to-create-an-ethereum-account/#how-to-create-an-ethereum-account

Screen Shot 2024-10-01 11 40 44 AM

@nloureiro
Copy link
Contributor

Another one centered

https://deploy-preview-13986--ethereumorg.netlify.app/en/guides/how-to-revoke-token-access/#step-1%3A-use-revoke-access-tools

And this tag... it hurts my eyes

Screen Shot 2024-10-01 12 06 28 AM

@nloureiro
Copy link
Contributor

there is a dot on the third line
https://deploy-preview-13986--ethereumorg.netlify.app/en/roadmap/beacon-chain/#what-is-the-beacon-chain
Screen Shot 2024-10-01 12 40 30 AM

@wackerow
Copy link
Member Author

wackerow commented Oct 2, 2024

Should the alert have some default margins to prevent this from happening?

Screen Shot 2024-10-01 11 28 46 AM

Agree we should fix this... Imo this shouldn't be done as a default Alert styling though. I'd like to handle this from the parent in a separate PR. The spacing here shines light on our article DOM structure which could use some improving (ie. use main instead of our current article and then wrap content below the h1's inside article, then style that template)

@wackerow
Copy link
Member Author

wackerow commented Oct 2, 2024

@wackerow, can we make the button collapse to the bottom on mobile? Or should it be on another PR?

Screen Shot 2024-10-01 11 15 18 AM

Played around with this... given it's current setup, and the complexity of this PR already, let's table to a separate task/PR.

@wackerow wackerow changed the title [WIP] InfoBanner to alert Deprecate InfoBanner, migrate usage to ui/alert components Oct 2, 2024
@wackerow
Copy link
Member Author

Juicy and unfortunate merge conflict with the latest translation import... will need to find time to go through this

@wackerow wackerow marked this pull request as draft October 18, 2024 14:41
@github-actions
Copy link
Contributor

This issue is stale because it has been open 30 days with no activity.

@github-actions github-actions bot added the Status: Stale This issue is stale because it has been open 30 days with no activity. label Nov 23, 2024
@wackerow
Copy link
Member Author

wackerow commented May 6, 2025

Closing due to complexity of merge conflicts; will likely be simpler to start fresh. Issue posted #15407; this branch can be used in whatever capacity as a reference for future implementation

@wackerow wackerow closed this May 6, 2025
@github-actions github-actions bot added the abandoned This has been abandoned or will not be implemented label May 6, 2025
@lukassim lukassim removed the Update Crowdin PR introduces changes that need to be updated in Crowdin label May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

abandoned This has been abandoned or will not be implemented content 🖋️ This involves copy additions or edits needs design approval 🧑‍🎨 Approval from a designer is needed before merging needs dev approval 🧑‍💻 Approval from a developer is needed before merging needs review 👀 Status: Stale This issue is stale because it has been open 30 days with no activity. translation 🌍 This is related to our Translation Program

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants