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

refactor(pages): migrate home page to Chakra #9364

Merged
merged 10 commits into from
Feb 20, 2023

Conversation

TylerAPfledderer
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer commented Jan 31, 2023

Description

Migrate the Home page to use Chakra UI

Related Issue

#9353

@github-actions github-actions bot added content 🖋️ This involves copy additions or edits review needed 👀 labels Jan 31, 2023
@gatsby-cloud
Copy link

gatsby-cloud bot commented Jan 31, 2023

✅ ethereum-org-website-dev deploy preview ready

@TylerAPfledderer TylerAPfledderer force-pushed the refactor/home-page-to-chakra branch from a9abf69 to 92c0e7f Compare February 1, 2023 02:12
@TylerAPfledderer TylerAPfledderer marked this pull request as ready for review February 2, 2023 01:16
@TylerAPfledderer TylerAPfledderer changed the title [WIP] refactor(pages): migrate home page to Chakra refactor(pages): migrate home page to Chakra Feb 2, 2023
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.

Nicely done @TylerAPfledderer! overall this looks great

padding: 2rem;
}
`
type ChildOnlyProp = { children: ReactNode }
Copy link
Member

Choose a reason for hiding this comment

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

This type looks like a good one to move to src/types.ts as I image we are going to start using it more often now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pettinarip Would you like to go ahead and move it in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, that would be great

p={{ sm: 8, lg: 24 }}
boxSize="full"
>
<SectionHeading fontFamily="inherit" mb={6}>
Copy link
Member

Choose a reason for hiding this comment

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

I think that all of our section headers should not have the margin top in order to have the content correctly centered. Perhaps we can add it directly on the SectionHeading component as its default.

Suggested change
<SectionHeading fontFamily="inherit" mb={6}>
<SectionHeading fontFamily="inherit" mb={6} mt={0}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied directly to the component

</MainSectionContainer>
{/* Eth Today Section */}
<GrayContainer>
<ContentBox>
<h2>
Copy link
Member

Choose a reason for hiding this comment

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

Lets use Headings for these h2s

<StyledContent>
</GrayContainer>
{/* Explore Section */}
<ContentBox>
<h2>
Copy link
Member

Choose a reason for hiding this comment

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

Here as well. Heading

<StyledContent>
</GrayContainer>
{/* Explore Section */}
<ContentBox>
<h2>
<Translation id="page-index-touts-header" />
</h2>
Copy link
Member

Choose a reason for hiding this comment

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

I think this entire h2 should be inside the below ContentBox section. If not this gets center aligned (and it should be left aligned).
image

<ButtonLink to="/dapps/?category=technology">
<Translation id="page-index-internet-button" />
</ButtonLink>
<StyledButtonLink variant="outline" to="/wallets/">
Copy link
Member

Choose a reason for hiding this comment

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

We missed this button

{/* Eth Today Section */}
<GrayContainer>
<ContentBox>
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Leveraging the SectionHeading component in replacing the h2 tag that was here and in the next section.

Since mt, mb, and fontFamily are defined in the component and overriding other styles specific to the h2 that are more preferable here, setting the values to undefined essentially cancels the overrides while still gaining the reuseability of the component.

Is this OK?

Copy link
Member

Choose a reason for hiding this comment

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

Don't like it tbh 😄

Lets do this for now. heading is the old theme font family.

Suggested change
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
<SectionHeading mt={12} mb={8} fontFamily="heading">

padding: 2rem;
}
`
type ChildOnlyProp = { children: ReactNode }
Copy link
Member

Choose a reason for hiding this comment

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

Yea, that would be great

{/* Eth Today Section */}
<GrayContainer>
<ContentBox>
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
Copy link
Member

Choose a reason for hiding this comment

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

Don't like it tbh 😄

Lets do this for now. heading is the old theme font family.

Suggested change
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
<SectionHeading mt={12} mb={8} fontFamily="heading">

{/* Explore Section */}
<ContentBox>
<Box pb={4}>
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Suggested change
<SectionHeading mt={undefined} mb={undefined} fontFamily={undefined}>
<SectionHeading mt={12} mb={8} fontFamily="heading">

@TylerAPfledderer
Copy link
Contributor Author

@pettinarip we should be good to go!

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.

@pettinarip pettinarip merged commit cd1e432 into ethereum:dev Feb 20, 2023
@TylerAPfledderer TylerAPfledderer deleted the refactor/home-page-to-chakra branch February 20, 2023 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived: chakra-migration content 🖋️ This involves copy additions or edits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants