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: Migrate StakingProductsCardGrid to Chakra #8679

Conversation

TylerAPfledderer
Copy link
Contributor

Description

Migrate the StakingProductsCardGrid component to Chakra UI

Side Note: This PR also fixes broken styling present in the current production.

Related Issue

#8632

@gatsby-cloud
Copy link

gatsby-cloud bot commented Nov 20, 2022

✅ ethereum-org-website-dev deploy preview ready

@minimalsm minimalsm added the high priority This has a high priority label Nov 21, 2022
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.

This looks great @TylerAPfledderer! left a comment.

maxH={24}
>
{!!Svg && <Icon as={Svg} fontSize="2rem" />}
<Heading fontSize="2xl" color="white" fontWeight="normal">
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 we want to keep the wider font style in here to keep the same styles we had before

Suggested change
<Heading fontSize="2xl" color="white" fontWeight="normal">
<Heading fontSize="2xl" color="white">

Copy link
Member

Choose a reason for hiding this comment

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

If this was implemented to fix the headers' height inconsistency I don't think this fixes all of them. We have larger texts like in this case:
image

I think we can tackle that specific issue on a different PR and/or discuss it with the design team as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Agreed, I can see a possible quick alteration with the grid to overcome this text wrapping, but it's definitely better with it's own PR and design team input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change of font weight was not intended to fix the header height. I removed the fontWeight prop.

}
}

const StakingPill: React.FC<{ type: string; 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.

Nice!

<ul>
</Flex>
<Box {...PADDED_DIV_STYLE} py={0}>
<List m={0} gap={3}>
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

@@ -564,11 +538,16 @@ const StakingProductCardGrid: React.FC<IProps> = ({ category }) => {
if (!rankedProducts) return null

return (
<CardGrid>
<SimpleGrid
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

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.

LGTM! thanks @TylerAPfledderer

@pettinarip pettinarip merged commit cd0c957 into ethereum:dev Nov 22, 2022
@TylerAPfledderer TylerAPfledderer deleted the refactor/staking-products-card-grid-chakra branch November 22, 2022 15:24
@corwintines corwintines mentioned this pull request Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chakra-migration high priority This has a high priority needs review 👀 Review is needed for this issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants