Skip to content

Migrate ExpandableInfo to Chakra #8224

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Oct 27, 2022
Merged

Migrate ExpandableInfo to Chakra #8224

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
23fe136
refactor(expandable-info): migrate to chakra
TylerAPfledderer Oct 11, 2022
f5b4062
style(expandable-info): fix fontSize/lineHeight for `Heading`
TylerAPfledderer Oct 13, 2022
7cb5c44
style(expandable-info): fix alignment of heading content
TylerAPfledderer Oct 13, 2022
1420c34
style(expandable-info): replace unit values with thematic values
TylerAPfledderer Oct 25, 2022
356d3d7
refactor(run-a-node): migrate the part with `ExpandableInfo` to Chakra
TylerAPfledderer Oct 25, 2022
ed72587
revert(expandable-info): revert from `forwardRef`
TylerAPfledderer Oct 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
333 changes: 136 additions & 197 deletions src/components/ExpandableInfo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,216 +1,155 @@
import React, { ReactNode, useState } from "react"
import styled from "@emotion/styled"
import { motion } from "framer-motion"
import React, { ReactNode } from "react"
import { GatsbyImage, IGatsbyImageData } from "gatsby-plugin-image"
import {
BackgroundProps,
Box,
Center,
ChakraProps,
Collapse,
forwardRef,
Heading,
HStack,
Icon,
Stack,
Text,
useDisclosure,
VStack,
} from "@chakra-ui/react"
import { motion } from "framer-motion"
import { MdExpandMore } from "react-icons/md"

import Icon from "./Icon"

const Card = styled.div<{
background: string
}>`
border: 1px solid ${({ theme }) => theme.colors.border};
border-radius: 2px;
padding: 1.5rem;
display: flex;
flex-direction: column;
margin-bottom: 1rem;
background: ${({ background, theme }) =>
background ? theme.colors[background] : theme.colors.background};
&:hover {
img {
transform: scale(1.08);
transition: transform 0.1s;
}
}
`

const Content = styled.div`
display: flex;
align-items: center;
justify-content: space-between;
gap: 3rem;
@media (max-width: ${({ theme }) => theme.breakpoints.m}) {
gap: 2rem;
flex-direction: column;
}
`

const TitleContent = styled.div`
display: flex;
align-items: center;
gap: 3rem;
width: 100%;
`

const Title = styled.h2`
margin-top: 0rem;
margin-bottom: 0.5rem;
`

const TextPreview = styled.p`
font-weight: 400;
color: ${(props) => props.theme.colors.text200};
margin-bottom: 0rem;
`

const Text = styled(motion.div)`
font-size: 16px;
font-weight: 400;
color: ${(props) => props.theme.colors.text};
margin-top: 2rem;
border-top: 1px solid ${(props) => props.theme.colors.border};
padding-top: 1.5rem;
`

const Question = styled.div`
margin-right: 1rem;
`

const Header = styled.div`
display: flex;
width: 100%;
margin: 1rem 0;
align-items: center;
img {
margin-right: 1.5rem;
}
`

const ButtonContainer = styled(motion.div)`
display: flex;
width: 5rem;
justify-content: center;
align-items: center;
min-height: 10rem;
cursor: pointer;
@media (max-width: ${({ theme }) => theme.breakpoints.m}) {
min-height: 100%;
height: 100%;
width: 100%;
margin: 0;
}
&:hover {
svg {
transform: scale(1.25);
transition: transform 0.1s;
}
}
`

export interface IProps {
export interface IProps extends ChakraProps {
children?: React.ReactNode
image?: IGatsbyImageData
title: ReactNode
contentPreview: ReactNode
background: string
forceOpen: boolean
background: BackgroundProps["background"]
forceOpen?: boolean
Copy link
Member

Choose a reason for hiding this comment

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

👍🏼

className?: string
}

const ExpandableInfo: React.FC<IProps> = ({
image,
title,
contentPreview,
children,
background,
forceOpen,
className,
}) => {
const [isVisible, setIsVisible] = useState(forceOpen)
const expandCollapse = {
collapsed: {
height: 0,
transition: {
when: "afterChildren",
},
},
expanded: {
height: "100%",
transition: {
when: "beforeChildren",
},
const ExpandableInfo = forwardRef<IProps, "div">(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped component in the Chakra forwardRef to allow Chakra props to be used on this component at the parent level.

See run-a-node page when migrating the styled component to Chakra.

Copy link
Member

Choose a reason for hiding this comment

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

Perfect. This could be something that we should enforce more on all of our components. To guarantee that we are forwarding the refs accordingly. What do you think?

Copy link
Contributor Author

@TylerAPfledderer TylerAPfledderer Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry @pettinarip . I made a grave error in my explanation and needed to review forwardRef!! 😅

So yes, the forwardRef from Chakra is an enhanced version of React's forwardRef (Check out the source code)

However, the primary purpose of this version of forwardRef is to ensure the as prop is passed in at the parent level without type error should the given component need to be a different html element or consume a different component on a particular instance.

For example, if this component normally renders a div wrapping the contents but one of the parent components needs it to be an article in a given instance.

So doing this:

<ExpandableInfo as="article">

Will not generate a type error.

See the extended documentation regarding the "as" prop

I now believe this is actually unnecessary here, because there is no negative impact to passing props at the parent level without forwardRef so long as the component props are typed correctly, no need for the as prop, and no need to pass reference at the parent level.

To go along with this topic, I'm aware of discussions regarding not to use React.FC because of the implicit typing, and I'm curious about your thoughts on this! 😄

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for the explanation & resource links again.

When we did the TS migration, I didn't know about that discussion about React.FC. Later, I saw that CRA was removing the usage of it and it grab my attention. I'm not super deep into the details but I can say that I don't see too much value on using it tbh and totally agree that the implicit children prop is not desired.

This is something we could discuss in a GH issue if you have a strong opinion about it and you feel that we should change it 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @pettinarip! I do not have a strong preference either way.

In fact current version of the FC does not type the children, so you have to explicitly type it either way!

The discussion is indeed old, and React 18 has helped out with this. The implementation of it for the purposes of these components has no impact either way, whether you do this:

const someComponent: React.FC<Type> = (props) =>

or this:

const someComponent = (props: Type) =>

But you would still have trouble using generic typing with it though, if any of the components were to use it.

I'm not aware if any of the components actually need that! 😄

I will revert back to React.FC from forwardRef as it is not needed here and the component will still accept incoming Chakra Props.

(
{
image,
title,
contentPreview,
children,
background,
forceOpen,
className,
...rest
},
}
const chevronFlip = {
collapsed: {
rotate: 0,
transition: {
duration: 0.1,
ref
) => {
const { isOpen, getButtonProps, getDisclosureProps } = useDisclosure({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to provide accessibility for this component, should toggling expansion be used (forceOpen = false). getButtonProps holds all the necessary aria-* attributes for toggling, including aria-expanded and aria-controls.

getDisclosureProps Provides id prop for aria-controls and the hidden attribute, which is needed in conjunction with the accordion pattern from the WAI-Aria Authoring Practices

This of course is on contingent on if accordion behavior should still exist.

Copy link
Member

Choose a reason for hiding this comment

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

This is great! thanks for the detailed explanation. Didn't know useDisclosure exported all those props for you, thats awesome 💪🏼

defaultIsOpen: forceOpen,
})

const chevronFlip = {
collapsed: {
rotate: 0,
transition: {
duration: 0.1,
},
},
},
expanded: {
rotate: 180,
transition: {
duration: 0.4,
expanded: {
rotate: 180,
transition: {
duration: 0.4,
},
},
},
}
const showHide = {
collapsed: {
display: "none",
},
expanded: {
display: "inline-block",
},
}
const fadeInOut = {
collapsed: {
opacity: 0,
transition: {
duration: 0.1,
},
},
expanded: {
opacity: 1,
transition: {
duration: 0.4,
},
},
}
return (
<Card background={background} className={className}>
<Content>
{image && <GatsbyImage image={image} alt="" />}
<TitleContent>
<Question>
<Header>
<Title>{title}</Title>
</Header>
<TextPreview>{contentPreview}</TextPreview>
</Question>
</TitleContent>
{!forceOpen && (
<ButtonContainer
variants={chevronFlip}
animate={isVisible ? "expanded" : "collapsed"}
initial={false}
onClick={() => setIsVisible(!isVisible)}
>
<Icon name="chevronDown" size="36" />
</ButtonContainer>
)}
</Content>
<motion.div
variants={expandCollapse}
animate={isVisible ? "expanded" : "collapsed"}
initial={false}
}

const animateToggle = isOpen ? "expanded" : "collapsed"

return (
<VStack
border="1px solid"
borderColor="border"
borderRadius="2px"
p="1.5rem"
mb="1rem"
Copy link
Member

Choose a reason for hiding this comment

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

In general, lets try to use as much as we can the Chakra scale system (for font sizes & spacing). To keep it consistent with the rest of the codebase and future design system.

Suggested change
p="1.5rem"
mb="1rem"
p={6}
mb={4}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bad habit of mine. I'll be sure to keep this in mind for future changes and any other existing PRs! 😄

spacing="0"
background={background ?? "background"}
position="relative"
_hover={{
"& img": {
transform: "scale(1.08)",
transition: "transform 0.1s",
},
}}
ref={ref}
{...rest}
>
<motion.div
variants={showHide}
animate={isVisible ? "expanded" : "collapsed"}
<Stack
justify="space-between"
gap={{ base: "2rem", md: "3rem" }}
flexDirection={{ base: "column", md: "row" }}
width="full"
>
{image && <GatsbyImage image={image} alt="" />}
<HStack gap="3rem" width="full">
<Box mr="1rem">
<HStack
width="full"
m="1rem 0"
sx={{
img: {
mr: "1.5rem",
},
}}
>
<Heading mt="0rem" mb="0.5rem" size="md" fontWeight="600">
{title}
</Heading>
</HStack>
<Text color="text200" mb="0">
{contentPreview}
</Text>
</Box>
</HStack>
{!forceOpen && (
<Center
as={motion.div}
variants={chevronFlip}
animate={animateToggle}
initial={false}
{...getButtonProps()}
width={{ base: "full", md: "5rem" }}
minHeight={{ base: "full", md: "10rem" }}
cursor="pointer"
_hover={{
"& svg": {
transform: "scale(1.25)",
transition: "transform 0.1s",
},
}}
>
<Icon as={MdExpandMore} boxSize="initial" size="36" />
</Center>
)}
</Stack>
<Collapse
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Transition effect simplified to the single Collapse component, as all the fade-in/out effects are apart of the collapse transition.

{...getDisclosureProps()}
in={isOpen}
startingHeight="0"
endingHeight="100%"
initial={false}
>
<Text
variants={fadeInOut}
animate={isVisible ? "expanded" : "collapsed"}
initial={false}
<Box
color="text"
mt="2rem"
borderTop="1px solid"
borderColor="border"
paddingTop="1.5rem"
>
{children}
</Text>
</motion.div>
</motion.div>
</Card>
)
}
</Box>
</Collapse>
</VStack>
)
}
)

export default ExpandableInfo