Refactor UpgradeTableOfContents component to use ChakraUI #7725
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| overflow-y: auto; | ||
| ` | ||
|
|
||
| const OuterList = styled(motion.ul)` |
There was a problem hiding this comment.
Motion ul wasn't doing anything? I've removed it.
There was a problem hiding this comment.
🤔 yeah, I don't see that we are doing any animation with it. Fine with removing it.
|
|
||
| const OuterList = styled(motion.ul)` | ||
| list-style-type: none; | ||
| list-style-image: none; |
There was a problem hiding this comment.
I have no idea what list-style-image was supposed to do, but it also appeared to be doing nothing when I tested removing it—so I just left it out as I wasn't sure the best way to approach this in Chakra.
There was a problem hiding this comment.
Yes, its fine to remove it since those styles are going to be covered by the Chakra component now.
I guess we had it here to do some kind of reset and make sure this doesn't do anything unexpected.
| m={0} | ||
| py={0} | ||
| ps={4} | ||
| pe={1} |
There was a problem hiding this comment.
Is always using the shorthand our preference?
There was a problem hiding this comment.
I would say so but I don't have a strong preference.
As a side note, in this particular case, we are going to style the native Chakra component (on the theme definition), as we did with the Button and the Link.
| py={0} | ||
| ps={4} | ||
| pe={1} | ||
| fontSize="1.25rem" |
There was a problem hiding this comment.
@pettinarip I couldn't find anything in the design system for 20px fonts.
There was a problem hiding this comment.
In this case, we should use xl.
xl === 1.25rem Here you can see the different values for fontSize https://chakra-ui.com/docs/styled-system/theme#typography
| pe={1} | ||
| fontSize="1.25rem" | ||
| fontWeight="normal" | ||
| lineHeight="1.6" |
There was a problem hiding this comment.
No variables for this, Chakra defaults don't cover 1.6.
There was a problem hiding this comment.
In this case, we should just use the raw value: 1.6rem.
There was a problem hiding this comment.
Another thought I had, could you try to remove it and test if it works? because we are defining in the global styles 1.6rem as the default.
| import { motion } from "framer-motion" | ||
| import { Link } from "gatsby" | ||
| import styled from "@emotion/styled" | ||
| import { Box, Link, ListItem, UnorderedList } from "@chakra-ui/react" |
There was a problem hiding this comment.
Am I correct in thinking that we import Link directly from Chakra as our Link component is already extending it?
There was a problem hiding this comment.
No, we should use our custom src/components/Link component since it uses Gatsby Link under the hood.
The Chakra Link will only have the same styles but not our custom logic.
| position: relative; | ||
| display: inline-block; | ||
| color: ${(props) => props.theme.colors.text300}; | ||
| margin-bottom: 0.5rem !important; |
There was a problem hiding this comment.
!important wasn't needed here, but I'm not clear how we'd implement it if it was.
There was a problem hiding this comment.
We shouldn't need to use !important with Chakra anymore since now, all the styles are props, and the way you override styles is by overriding those props.
Example:
// our custom Link component
function Link(props) {
return <ChakraLink p={4} {...props} />
}
// overriding styles
<Link p={0} />
✅ ethereum-org-website-dev deploy preview ready
|
minimalsm
requested review from
corwintines,
jmcook1186,
pettinarip,
samajammin and
wackerow
as code owners
pettinarip
left a comment
pettinarip
left a comment
There was a problem hiding this comment.
Good job! I've replied to your comments with some recommendations. LMK what do you think 👍🏼
| import { motion } from "framer-motion" | ||
| import { Link } from "gatsby" | ||
| import styled from "@emotion/styled" | ||
| import { Box, Link, ListItem, UnorderedList } from "@chakra-ui/react" |
There was a problem hiding this comment.
No, we should use our custom src/components/Link component since it uses Gatsby Link under the hood.
The Chakra Link will only have the same styles but not our custom logic.
|
|
||
| const OuterList = styled(motion.ul)` | ||
| list-style-type: none; | ||
| list-style-image: none; |
There was a problem hiding this comment.
Yes, its fine to remove it since those styles are going to be covered by the Chakra component now.
I guess we had it here to do some kind of reset and make sure this doesn't do anything unexpected.
| overflow-y: auto; | ||
| ` | ||
|
|
||
| const OuterList = styled(motion.ul)` |
There was a problem hiding this comment.
🤔 yeah, I don't see that we are doing any animation with it. Fine with removing it.
| m={0} | ||
| py={0} | ||
| ps={4} | ||
| pe={1} |
There was a problem hiding this comment.
I would say so but I don't have a strong preference.
As a side note, in this particular case, we are going to style the native Chakra component (on the theme definition), as we did with the Button and the Link.
| py={0} | ||
| ps={4} | ||
| pe={1} | ||
| fontSize="1.25rem" |
There was a problem hiding this comment.
In this case, we should use xl.
xl === 1.25rem Here you can see the different values for fontSize https://chakra-ui.com/docs/styled-system/theme#typography
| pe={1} | ||
| fontSize="1.25rem" | ||
| fontWeight="normal" | ||
| lineHeight="1.6" |
There was a problem hiding this comment.
In this case, we should just use the raw value: 1.6rem.
| <OuterList> | ||
| <Box | ||
| as="aside" | ||
| className={className} |
There was a problem hiding this comment.
I don't think we are passing any className to this component in any of the uses. I would recommend removing it since we should handle all the styles using props.
| {items.map((item, index) => ( | ||
| <ListItem key={index}> | ||
| <ListItem margin={0} key={index}> | ||
| <div> |
Description
Migrate UpgradeTableOfContents from styled component to ChakraUI
Related Issue
#6374