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.

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

Migrate layer 2 page to shadcn, and updates for layer 2 migration #13987

Merged
merged 15 commits into from
Oct 29, 2024

Conversation

corwintines
Copy link
Member

@corwintines corwintines commented Sep 27, 2024

Description

Migrates layer 2 page to shadcn
Move layer 2 page to /layer-2/learn
Updates for the layer 2 migration to this page

Link: https://deploy-preview-13987--ethereumorg.netlify.app/layer-2/learn

Copy link

netlify bot commented Sep 27, 2024

Deploy Preview for ethereumorg failed.

Name Link
🔨 Latest commit b1e85a7
🔍 Latest deploy log https://app.netlify.com/sites/ethereumorg/deploys/66ff5f632cb6e600080f267b

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program labels Sep 27, 2024
Copy link
Member

@wackerow wackerow left a comment

Choose a reason for hiding this comment

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

Nice @corwintines! Lookin good, just dropped an initial round of feedback and thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

image

Noticing this is an image the chops on the top and bottom edges... looks a little strange having padding in the Y-direction here inside the hero cc: @nloureiro

src/components/Callout.tsx Outdated Show resolved Hide resolved
Comment on lines +54 to +69
{titleKey && (
<h3 className="mb-8 mt-10 text-2xl leading-[1.4]">{t(titleKey)}</h3>
)}
{title && (
<h3 className="mb-8 mt-10 text-2xl leading-[1.4]">{title}</h3>
)}
{descriptionKey && (
<p className="mb-6 text-xl leading-[140%] text-body-medium">
{t(descriptionKey)}
</p>
)}
{description && (
<p className="mb-6 text-xl leading-[140%] text-body-medium">
{description}
</p>
)}
Copy link
Member

@wackerow wackerow Sep 30, 2024

Choose a reason for hiding this comment

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

I'm a little unclear why we're opening up the props to optionally accept title and description or the keys for these:

image

Is the plan to change these components to not have to handle the translation keys? As-is we no longer have any of the props as required.

Comment on lines 172 to 174
if (path.startsWith("/layer-2/")) {
primaryNamespace = "page-layer-2"
}
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that as-is the page-layer-2 namespace will be loaded on /layer-2/learn/; is this what we want? If not, I'd change this one to not use startsWith()

Comment on lines +2 to +7
"page-layer-2-learn-meta-title": "What is layer 2",
"page-layer-2-learn-title": "What is layer 2",
"page-layer-2-learn-description": "Scaling Ethereum for mass adoption",
"page-layer-2-learn-button-1-label": "What is layer 2",
"page-layer-2-learn-button-2-label": "Use layer 2",
"page-layer-2-learn-what-is-layer-2-title": "What is layer 2?",
Copy link
Member

Choose a reason for hiding this comment

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

Seeing 4 instances of "What is layer 2"... one of which has a question mark

  1. Should we have a question mark at the end of this string? (I think we should)
  2. Do we need to repeat this key, or can we simplify to just have one instance of "What is layer 2?"

defihook

This comment was marked as spam.

@corwintines corwintines merged commit d04ebfc into l2revamp Oct 29, 2024
4 of 10 checks passed
@corwintines corwintines deleted the l2Learn branch October 29, 2024 04:36
Copy link

@defihook defihook left a comment

Choose a reason for hiding this comment

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

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content 🖋️ This involves copy additions or edits tooling 🔧 Changes related to tooling of the project translation 🌍 This is related to our Translation Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants