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

migrated: pages/developers/learning-tools.tsx to Chakra UI #10348

Merged
merged 4 commits into from
Jun 19, 2023

Conversation

Seek4samurai
Copy link
Contributor

Description

Migrated pages/developers/learning-tools.tsx to Chakra UI.

Related Issue

#9353

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 29, 2023

✅ ethereum-org-website-dev deploy preview ready

Copy link
Contributor

@minimalsm minimalsm left a comment

Choose a reason for hiding this comment

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

Hey @Seek4samurai, nice work on this 💪

I've Made some suggested tweaks to get this closer to the current design.

The main thing we need to solve is that we are setting some breakpoints the wrong way round (e.g. 0px margin on big screens and 8px on small, instead of the opposite). I've left suggestions that I believe fixes all of these 🫡

src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
src/pages/developers/learning-tools.tsx Outdated Show resolved Hide resolved
const Subtitle = (props: ChildOnlyProp) => (
<Flex
fontSize={"xl"}
lineHeight={"short"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noting, this is a tiny bit different from what we have currently. Not sure if we care about this @pettinarip?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, we want to keep the current styles so this should be lineHeight={1.4}

Copy link
Contributor

@minimalsm minimalsm left a comment

Choose a reason for hiding this comment

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

Hey @Seek4samurai, nice work on this 💪

I've Made some suggested tweaks to get this closer to the current design.

The main thing we need to solve is that we are setting some breakpoints the wrong way round (e.g. 0px margin on big screens and 8px on small, instead of the opposite). I've left suggestions that I believe fixes all of these 🫡

@Seek4samurai
Copy link
Contributor Author

Ok maybe I did messed up some breakpoints. Thanks!

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.

Thanks for the PR @Seek4samurai.

I've applied the suggestions from the reviews. This is good to go now 💪🏼

@pettinarip pettinarip requested a review from minimalsm June 19, 2023 21:22
@pettinarip pettinarip dismissed minimalsm’s stale review June 19, 2023 21:23

Suggestions were applied. Will merge it now. Feel free to add more comments if needed

@pettinarip pettinarip merged commit af354c3 into ethereum:dev Jun 19, 2023
This was referenced Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants