Skip to content

Conversation

@shariqanwar20
Copy link
Contributor

Description

The component has been migrated to chakra UI
The emotion styled component has been removed from the code

  • Facing issue in centering Cash Emoji when screen size is decreased

  • Each emotion component is replaced with inline style attributes using chakra UI

  • Media queries have been handled using breakpoints in chakra

Before

Screenshot (9)
Screenshot (10)
Screenshot (8)

After

Screenshot (12)
Screenshot (11)
Screenshot (8)

Related Issue

Refs: #6374

@gatsby-cloud
Copy link

gatsby-cloud bot commented Oct 7, 2022

✅ ethereum-org-website-dev deploy preview ready

@shariqanwar20 shariqanwar20 changed the title [UI] Stablecoin Box Grid: migrate component to chakra UI (Need help in centering cash emoji) [UI] Stablecoin Box Grid: migrate component to chakra UI Oct 7, 2022
Copy link
Contributor

@TylerAPfledderer TylerAPfledderer left a comment

Choose a reason for hiding this comment

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

Hey @shariqanwar20! Nice work! I had a chance to look through your PR and left some comments. 😃

I think there are many spots where reusable child components should be recreated (Like what was originally labeled Subtitle, Row, Column, etc. to make sure the code is DRY. 😸

gridColumnStart={isOpen ? columnNumber : `auto`}
color={isOpen ? "black300" : "text"}
cursor={isOpen ? `auto` : `pointer`}
background={isOpen ? `${color}` : "background"}
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency throughout the file, I suggest removing the interpolation around the color prop :)

Comment on lines 121 to 127
<Text
fontSize={{ base: "2rem", sm: "2.5rem" }}
fontWeight={700}
marginTop={0}
>
{title}
</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

Also needs to be Heading with an h3 similar to the previous.

Copy link
Member

Choose a reason for hiding this comment

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

Correct

Comment on lines 88 to 90
background: `${isOpen ? color : "ednBackground"}`,
transition: `${isOpen ? "auto" : "transform 0.5s"}`,
transform: `${isOpen ? "auto" : "skewX(-5deg)"}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Interpolation is not needed here in order for the values to be rendered correctly

Comment on lines 109 to 115
<Text
fontSize={{ base: "2rem", sm: "2.5rem" }}
fontWeight={400}
marginTop={0}
>
{title}
</Text>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the Heading component, as it is an h3 (needs as='h3') but is currently being rendered as a p.

Additional props are required in the interim to ensure it still renders as the original.

Copy link
Member

Choose a reason for hiding this comment

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

Correct. Lets fix this

<Column>
<Subtitle>
<Box w="100%">
<Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be a Heading with an h4 similar to previous

<Column>
<Subtitle>
<Box w="100%">
<Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be a Heading with an h4 similar to previous

</Flex>
<div>
<Subtitle>
<Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Also should be a Heading with an h4 similar to previous

Copy link
Member

Choose a reason for hiding this comment

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

Same

gridTemplateColumns={"3fr 1fr"}
gridTemplateRows={"3fr 3fr"}
borderRadius="2px"
margin={0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be margin="4rem 0" to have the vertical spacing around the container.

Copy link
Member

Choose a reason for hiding this comment

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

We could just do my={16} in this case to use the chakra spacing values

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.

Good job @shariqanwar20

Comment on lines 109 to 115
<Text
fontSize={{ base: "2rem", sm: "2.5rem" }}
fontWeight={400}
marginTop={0}
>
{title}
</Text>
Copy link
Member

Choose a reason for hiding this comment

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

Correct. Lets fix this

Comment on lines 121 to 127
<Text
fontSize={{ base: "2rem", sm: "2.5rem" }}
fontWeight={700}
marginTop={0}
>
{title}
</Text>
Copy link
Member

Choose a reason for hiding this comment

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

Correct

</Flex>
<div>
<Subtitle>
<Text
Copy link
Member

Choose a reason for hiding this comment

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

Same

gridTemplateColumns={"3fr 1fr"}
gridTemplateRows={"3fr 3fr"}
borderRadius="2px"
margin={0}
Copy link
Member

Choose a reason for hiding this comment

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

We could just do my={16} in this case to use the chakra spacing values

<Container
gridTemplateColumns={"3fr 1fr"}
gridTemplateRows={"3fr 3fr"}
borderRadius="2px"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
borderRadius="2px"
borderRadius="sm"

<Text
fontSize={{ base: "1.5rem", sm: "2rem" }}
fontWeight={600}
marginTop="0rem"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
marginTop="0rem"
marginTop="0"

fontSize={{ base: "1.5rem", sm: "2rem" }}
fontWeight={600}
marginTop="0rem"
padding="0.5rem"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
padding="0.5rem"
padding={2}

fontWeight={600}
marginTop="0rem"
padding="0.5rem"
paddingBottom="1rem"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
paddingBottom="1rem"
paddingBottom={4}

@shariqanwar20
Copy link
Contributor Author

@pettinarip @TylerAPfledderer Thanks for the feedback. I will start working on the changes right away

@shariqanwar20 shariqanwar20 requested review from TylerAPfledderer and pettinarip and removed request for TylerAPfledderer and pettinarip October 31, 2022 08:12
@shariqanwar20 shariqanwar20 requested review from TylerAPfledderer and pettinarip and removed request for TylerAPfledderer and pettinarip October 31, 2022 08:19
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 @shariqanwar20! I've pushed some minor fixes but overall this looks great.

Be sure to join the discord if you are interested in contributing further to the project or have any questions for the team. And we've just released our 2022 POAPs so remember to claim yours also 🥳!

@pettinarip pettinarip merged commit 01c9d00 into ethereum:dev Oct 31, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Oct 31, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 Ethereum.org Contributor:

GitPOAP: 2022 Ethereum.org Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@pettinarip
Copy link
Member

@all-contributors please add @shariqanwar20 for code

@allcontributors
Copy link
Contributor

@pettinarip

I've put up a pull request to add @shariqanwar20! 🎉

@pettinarip pettinarip mentioned this pull request Oct 31, 2022
80 tasks
@corwintines corwintines mentioned this pull request Oct 31, 2022
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