Skip to content

Conversation

nikkhielseath
Copy link
Contributor

Migrating styles from Styled Components to Chakra UI

Description

Refactor the ActionCard component to use
the components provided by Chakra UI

Related Issue

@nikkhielseath nikkhielseath changed the title Refactor/#6374 action card Chakra UI: Migrate Action card Sep 24, 2022
@gatsby-cloud
Copy link

gatsby-cloud bot commented Sep 24, 2022

✅ ethereum-org-website-dev deploy preview ready

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.

Great job @SNikhill 💪🏼

<ImageWrapper
isRight={isRight}
isBottom={isBottom}
<Link
Copy link
Member

Choose a reason for hiding this comment

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

Instead of wrapping this component with a Link, lets use the LinkOverlay component that Chakra offers for this specific scenario https://chakra-ui.com/docs/components/link-overlay/usage

I did a similar implementation in #7721

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I see. So, if I understand the usage correctly, the whole card will be wrapped in a LinkBox and then, I can just wrap the heading in a LinkOverlay
And the LinkBox will elevate this link in a way that to the user it looks like the whole card is a link.

transform: scale(1.02);
}
`
const LinkFocusStyles = {
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
const LinkFocusStyles = {
const linkFocusStyles = {

`
const LinkFocusStyles = {
textDecoration: "none",
borderRadius: "4px",
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: "4px",
borderRadius: "base",

Comment on lines 70 to 80
minH={"260px"}
bg={"cardGradient"}
direction={"row"}
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
minH={"260px"}
bg={"cardGradient"}
direction={"row"}
minH="260px"
bg="cardGradient"
direction="row"

direction={"row"}
justify={isRight ? "flex-end" : "center"}
align={isBottom ? "flex-end" : "center"}
className="action-card-image-wrapper"
Copy link
Member

Choose a reason for hiding this comment

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

className="action-card-image-wrapper"
boxShadow="inset 0px -1px 0px rgba(0, 0, 0, 0.1)"
>
{!isImageURL && <Image image={image} alt={alt || ""} />}
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 the Image from Chakra. https://chakra-ui.com/docs/components/image/usage

<Image as="GatsbyImage" ...>

</Content>
</Card>
</Flex>
<Box p={6} className="action-card-content">
Copy link
Member

Choose a reason for hiding this comment

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

@pettinarip pettinarip self-assigned this Sep 28, 2022
@nikkhielseath nikkhielseath force-pushed the refactor/#6374-action-card branch from ea95b32 to 1fb7316 Compare November 5, 2022 19:10
@nikkhielseath nikkhielseath force-pushed the refactor/#6374-action-card branch from fcd4c05 to 70159a0 Compare November 5, 2022 19:53
Copy link
Contributor Author

@nikkhielseath nikkhielseath left a comment

Choose a reason for hiding this comment

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

I have made the suggested changes.

That LinkBox and LinkOverlay thing is good but, I have left a question regarding the same in that section.

</Flex>
<Box p={6} className="action-card-content">
<Heading as="h3" fontSize="2xl" mt={2} mb={4}>
<LinkOverlay
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapping the title with LinkOverlay based on the suggested refactor

<ImageWrapper
isRight={isRight}
isBottom={isBottom}
<LinkBox
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have divided the Link styles between LinkBox and Overlay.

alt={alt || ""}
as={GatsbyImage}
maxH="257px"
maxW={{ base: "372px", sm: "311px" }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, as suggested, I am using Image. The breakpoint thing was nice to know about. Thank you for mentioning that in the Migration Guide. But, just to inform you the example the in Migration Guide is inversed.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@SNikhill keep in mind here that in this example you provided and the Image with styled components are using a max-width media query, but when using Chakra tokens by default use min-width. So the example in the migration guide is actually correct! 😄

Therefore, what you provided needs to be reversed.

Suggested change
maxW={{ base: "372px", sm: "311px" }}
maxW={{ base: "311px", sm: "372px" }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, okay. Thank you for explaining that.

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 @SNikhill !

Following up on this PR as it has grown a little stale. I've provided some thoughts. 😸

An aside: @pettinarip It appears that this component has an existing broken layout (Check at roughly 414px - 500px screen widths). Probably save a fix for post migration in a separate PR?

alt={alt || ""}
as={GatsbyImage}
maxH="257px"
maxW={{ base: "372px", sm: "311px" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@SNikhill keep in mind here that in this example you provided and the Image with styled components are using a max-width media query, but when using Chakra tokens by default use min-width. So the example in the migration guide is actually correct! 😄

Therefore, what you provided needs to be reversed.

Suggested change
maxW={{ base: "372px", sm: "311px" }}
maxW={{ base: "311px", sm: "372px" }}

{title}
</LinkOverlay>
</Heading>
<Text mb={0} opacity={0.8}>
Copy link
Contributor

Choose a reason for hiding this comment

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

opacity will break the LinkBox behavior when hovering over this text due to CSS Stacking context, resulting in the text not being clickable. I suggest the following to maintain accessibility with color contrast.

This requires the use of the useColorModeValue hook from Chakra

Before the component return:

const descriptionColor = useColorModeValue("blackAlpha.700", "whiteAlpha.800")

and then...

Suggested change
<Text mb={0} opacity={0.8}>
<Text mb={0} color={descriptionColor}>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the information! I have made the change.

@pettinarip pettinarip mentioned this pull request Jan 18, 2023
80 tasks
@gitpoap-bot
Copy link

gitpoap-bot bot commented Mar 27, 2023

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

Be sure to join the Ethereum.org discord if you are interested in contributing further to the project or have any questions for the team.

GitPOAP: 2023 Ethereum.org Contributor:

GitPOAP: 2023 Ethereum.org Contributor GitPOAP Badge

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

Learn more about GitPOAPs here.

@corwintines corwintines mentioned this pull request Mar 29, 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.

4 participants