Migrate `ExpandableInfo` to Chakra #8224

TylerAPfledderer · 2022-10-11T02:02:18Z

Description

Per discussion in Discord, there was the question about using the Accordion prop for this component, but did not make sense to do so for accessibility because of it's current usage as only a forced-open iteration on a single page. So going by the existing functionality was considered the best course of action.

See comments below for explanation on certain Chakra additions.

Related Issue

#6374

TylerAPfledderer · 2022-10-11T02:05:18Z

src/components/ExpandableInfo.tsx

-      transition: {
-        when: "beforeChildren",
-      },
+const ExpandableInfo = forwardRef<IProps, "div">(


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.

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?

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! 😄

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 👍🏼

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.

TylerAPfledderer · 2022-10-11T02:28:48Z

src/components/ExpandableInfo.tsx

-        duration: 0.1,
+    ref
+  ) => {
+    const { isOpen, getButtonProps, getDisclosureProps } = useDisclosure({


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.

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

gatsby-cloud · 2022-10-11T02:41:10Z

✅ ethereum-org-website-dev deploy preview ready

Deploy preview
Build logs · 19m build time

TylerAPfledderer · 2022-10-13T23:30:22Z

src/components/ExpandableInfo.tsx

+            </Center>
+          )}
+        </Stack>
+        <Collapse


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

pettinarip

This looks great @TylerAPfledderer nice job 💪🏼

I haven't tested the collapsing feature yet but one thing I've noticed is that in the https://ethereum.org/en/run-a-node/ page we have a width=90% that is not being applied now. Would suggest replacing the emotion component that is wrapping this component in that page with a chakra approach.

// current run-a-node.tsx page implementation
const StyledExpandableInfo = styled(ExpandableInfo)`
  width: 90%;
  align-self: center;
  @media (max-width: ${({ theme }) => theme.breakpoints.m}) {
    margin: 0;
    width: 100%;
  }
`

// migrate this to chakra as well
// now it would be something like
<ExpandableInfo
  ...
  width="90%"
>

pettinarip · 2022-10-24T14:57:56Z

src/components/ExpandableInfo.tsx

-        duration: 0.1,
+    ref
+  ) => {
+    const { isOpen, getButtonProps, getDisclosureProps } = useDisclosure({


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

pettinarip · 2022-10-24T14:59:53Z

src/components/ExpandableInfo.tsx

-      transition: {
-        when: "beforeChildren",
-      },
+const ExpandableInfo = forwardRef<IProps, "div">(


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?

pettinarip · 2022-10-24T18:44:45Z

src/components/ExpandableInfo.tsx

-  background: string
-  forceOpen: boolean
+  background: BackgroundProps["background"]
+  forceOpen?: boolean


pettinarip · 2022-10-24T18:48:01Z

src/components/ExpandableInfo.tsx

+        p="1.5rem"
+        mb="1rem"


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}

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

TylerAPfledderer · 2022-10-24T22:55:10Z

As to the width issue, I will go ahead an migrate the instance over in the run-a-node page! 👍🏼

TylerAPfledderer · 2022-10-24T23:49:44Z

@pettinarip to add to the issue with the styling from the run-a-node page...

There is also a margin: 0 applied below the md breakpoint.

There are a couple of [cleaner] approaches to ensure this is rendered properly and not lose the mb styling that is in the component.

Explicit responsive values: mb={{ base: 0, md: 4 }} (you have to restate the original value of 4 else it will be overridden)
Use the useQuery() hook from Chakra and apply to the sx prop, like so...

// Somewhere before the return
const mdBelowQuery = useQuery({ below: 'md' })

//Then on the component
<ExpandableInfo
  sx={{
    [`@media ${mdBelowQuery}`] : {
      mb: 0
    }
>

Which under the hood will render:

@media (max-width: 48em) {
  margin-bottom: 0
}

The current problem I see is that the string from useQuery is not being returned correctly (returning (max-width: md), and I'm trying to get some help to troubleshoot the issue). But if it was working correctly, might be useful for other components on this page.

Let me know what you think about this!

TylerAPfledderer · 2022-10-25T00:11:35Z

src/components/ExpandableInfo.tsx

+                <Heading
+                  mt="0"
+                  mb={1}
+                  fontSize={{ base: "2xl", md: "2rem" }}


If you replaced md: "2rem" with md: 8 here, it will render 8px because the fontSize prop does not like it.

Oh got it. My bad haha thought it was a spacing prop 🤪

pettinarip · 2022-10-25T21:44:52Z

@TylerAPfledderer thanks for the explanation. I think I prefer the most common approach for now (mb={{ base: 0, md: 4 }}). The other one seems to verbose to me.

TylerAPfledderer · 2022-10-25T22:06:28Z

@TylerAPfledderer thanks for the explanation. I think I prefer the most common approach for now (mb={{ base: 0, md: 4 }}). The other one seems to verbose to me.

I agree! 😅

TylerAPfledderer · 2022-10-25T23:54:37Z

@pettinarip should be good to go now with all changes!

pettinarip

lgtm! thanks @TylerAPfledderer

refactor(expandable-info): migrate to chakra

23fe136

github-actions bot added the review needed 👀 label Oct 11, 2022

TylerAPfledderer changed the title ~~refactor(expandable-info): migrate to chakra~~ Migrate ExpandableInfo to Chakra Oct 11, 2022

TylerAPfledderer commented Oct 11, 2022

View reviewed changes

TylerAPfledderer marked this pull request as ready for review October 11, 2022 02:43

TylerAPfledderer requested review from corwintines, jmcook1186, minimalsm, pettinarip, samajammin and wackerow as code owners October 11, 2022 02:43

pettinarip added the archived: chakra-migration label Oct 12, 2022

TylerAPfledderer added 2 commits October 13, 2022 19:17

style(expandable-info): fix fontSize/lineHeight for Heading

f5b4062

style(expandable-info): fix alignment of heading content

7cb5c44

TylerAPfledderer commented Oct 13, 2022

View reviewed changes

pettinarip requested changes Oct 24, 2022

View reviewed changes

style(expandable-info): replace unit values with thematic values

1420c34

TylerAPfledderer commented Oct 25, 2022

View reviewed changes

refactor(run-a-node): migrate the part with ExpandableInfo to Chakra

356d3d7

github-actions bot added the content 🖋️ This involves copy additions or edits label Oct 25, 2022

revert(expandable-info): revert from forwardRef

ed72587

pettinarip approved these changes Oct 27, 2022

View reviewed changes

pettinarip merged commit 0b5f6f9 into ethereum:dev Oct 27, 2022

pettinarip mentioned this pull request Oct 27, 2022

Epic: Implement UI library #6374

Closed

80 tasks

TylerAPfledderer deleted the feat/chakra-migrate-expandable-info branch October 27, 2022 20:46

corwintines mentioned this pull request Oct 27, 2022

Deploy v6.8.0 #8390

Merged

Uh oh!

Migrate ExpandableInfo to Chakra #8224

Migrate ExpandableInfo to Chakra #8224

Uh oh!

Conversation

TylerAPfledderer commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerAPfledderer Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatsby-cloud bot commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ ethereum-org-website-dev deploy preview ready

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pettinarip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerAPfledderer commented Oct 24, 2022

Uh oh!

TylerAPfledderer commented Oct 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pettinarip commented Oct 25, 2022

Uh oh!

TylerAPfledderer commented Oct 25, 2022

Uh oh!

TylerAPfledderer commented Oct 25, 2022

Uh oh!

pettinarip left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Migrate `ExpandableInfo` to Chakra #8224

Migrate `ExpandableInfo` to Chakra #8224

TylerAPfledderer commented Oct 11, 2022 •

edited

Loading

TylerAPfledderer Oct 24, 2022 •

edited

Loading

gatsby-cloud bot commented Oct 11, 2022 •

edited

Loading

TylerAPfledderer commented Oct 24, 2022 •

edited

Loading