refactor: migrate the merge infographic component from emotion to chakraui #6374 #8246

victorpatru · 2022-10-13T17:22:59Z

Hey @pettinarip, I've gone ahead and fixed the bugs that I encountered during the migration of the MergeInfographic component.

I wasn't sure about the best practices to fixing the bugs to a pull request so I closed the previous pull request and created a new one with my final version. (would love some insight on whether this was the right approach).

Related Issue #6374

…kraui

gatsby-cloud · 2022-10-13T18:00:13Z

✅ ethereum-org-website-dev deploy preview ready

Deploy preview
Build logs · 25m build time

TylerAPfledderer

Hey @victorpatru ! I got a chance to take a look at your changes and provided some feedback for you 😄

TylerAPfledderer · 2022-10-22T11:32:18Z

src/components/MergeInfographic.tsx

+    <VStack
      className={className}
      role="img"
      aria-label={translateMessageId(
        "page-upgrades-merge-infographic-alt-text",
        intl
      )}
+      position="relative"
+      width="100%"
+      sx={{
+        isolation: "isolate",
+        aspectRatio: `${25 / 11}`,
+      }}
    >


Because the aspectRatio prop is used, I think it would be better to use the AspectRatio component instead, else Box.

VStack doesn't make sense here because there are no visually stacking elements in the DOM flow (just about everything is absolute-positioned instead), and VStack by default renders a marginTop for each child element. In this case, it's simply rendering unnecessary space. :)

With AspectRatio you can use the ratio prop instead with the needed value.

NOTE: All the child components will need to be wrapped in a div because AspectRatio is looking for a single child. Else it'll break the SSR.

Suggested change

<VStack

className={className}

role="img"

aria-label={translateMessageId(

"page-upgrades-merge-infographic-alt-text",

intl

)}

position="relative"

width="100%"

sx={{

isolation: "isolate",

aspectRatio: `${25 / 11}`,

}}

>

<AspectRatio

className={className}

role="img"

aria-label={translateMessageId(

"page-upgrades-merge-infographic-alt-text",

intl

)}

position="relative"

width="100%"

ratio={25 / 11}

sx={{

isolation: "isolate",

}}

>

This component will provide more robust positioning and sizing logic. I wonder if @pettinarip agrees on this approach instead of just using Box! 😄

Great suggestion, yes. Lets try to use as much as we can the Chakra components. And good point about the usage of VStack, I think its not necessary here.

TylerAPfledderer · 2022-10-22T11:35:43Z

src/components/MergeInfographic.tsx

-      <ExecutionLayer aria-hidden="true">
+      <Box
+        position="absolute"
+        top="44%"


top should have the value of 40% here.

That is weird. You are correct, in the code we have 40% but in prod, inspecting the element using the dev tools, it says 44% 🤷🏼 whatever...both values work fine at the end IMO.

TylerAPfledderer · 2022-10-22T11:47:56Z

src/components/MergeInfographic.tsx

      aria-hidden="true"
    >
-      <Text x="2%" y="35%" fontSize={lg}>
+      <text x="2%" y="35%" fontSize={lg} textAnchor="start" fill="currentColor">


With all of the text elements, you could do the same as the Background svg in using the Chakra factory, so you can keep the code DRY with the textAnchor and fill props.

const Text = chakra("text", { baseStyle: { textAnchor: "start", fill: "currentColor", }, })

pettinarip

Great job @victorpatru! this is looking great.

Lets implement the AspectRatio component in this refactor. With that, I think we can call this done.

pettinarip · 2022-10-25T14:16:27Z

src/components/MergeInfographic.tsx

+    <VStack
      className={className}
      role="img"
      aria-label={translateMessageId(
        "page-upgrades-merge-infographic-alt-text",
        intl
      )}
+      position="relative"
+      width="100%"
+      sx={{
+        isolation: "isolate",
+        aspectRatio: `${25 / 11}`,
+      }}
    >


Great suggestion, yes. Lets try to use as much as we can the Chakra components. And good point about the usage of VStack, I think its not necessary here.

pettinarip · 2022-10-25T14:19:17Z

src/components/MergeInfographic.tsx

-      <ExecutionLayer aria-hidden="true">
+      <Box
+        position="absolute"
+        top="44%"


That is weird. You are correct, in the code we have 40% but in prod, inspecting the element using the dev tools, it says 44% 🤷🏼 whatever...both values work fine at the end IMO.

victorpatru · 2022-10-25T14:37:53Z

Committed the changes we've discussed. Thanks @pettinarip @TylerAPfledderer

TylerAPfledderer · 2022-10-25T15:30:40Z

src/components/MergeInfographic.tsx

      aria-hidden="true"
    >
-      <Text x="2%" y="35%" fontSize={lg}>
+      <Text x="2%" y="35%" fontSize={lg} textAnchor="start" fill="currentColor">


Whoops! Straggler textAnchor and fill after making that new component. 😄

victorpatru · 2022-10-25T18:24:36Z

The build kept failing because the AspectRatio Chakra element expects a single child element. It should be fixed now 😄

pettinarip

LGTM! thanks for the great work you did here @victorpatru 🙏🏼

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

gitpoap-bot · 2022-10-29T15:40:18Z

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

GitPOAP: 2022 Ethereum.org Contributor:

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

Learn more about GitPOAPs here.

pettinarip · 2022-10-29T15:40:31Z

@all-contributors please add @victorpatru for code

allcontributors · 2022-10-29T15:40:40Z

@pettinarip

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

refactor: migrate the merge infographic component from emotion to cha…

2f85327

…kraui

victorpatru requested review from corwintines, jmcook1186, minimalsm, pettinarip, samajammin and wackerow as code owners October 13, 2022 17:23

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

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

victorpatru changed the title ~~refactor: migrate the merge infographic component from emotion to chakraui~~ refactor: migrate the merge infographic component from emotion to chakraui #6374 Oct 16, 2022

TylerAPfledderer suggested changes Oct 22, 2022

View reviewed changes

pettinarip requested changes Oct 25, 2022

View reviewed changes

fix: incorporating pr feedback

b9d37bb

TylerAPfledderer reviewed Oct 25, 2022

View reviewed changes

victorpatru added 2 commits October 25, 2022 21:10

fix: text attribute bug

be51474

fix: aspect ratio container expects only one children

c7e37e1

pettinarip approved these changes Oct 29, 2022

View reviewed changes

pettinarip merged commit d7b54e8 into ethereum:dev Oct 29, 2022

allcontributors bot mentioned this pull request Oct 29, 2022

docs: add victorpatru as a contributor for code #8412

Merged

pettinarip mentioned this pull request Oct 29, 2022

Epic: Implement UI library #6374

Closed

80 tasks

corwintines mentioned this pull request Oct 31, 2022

Deploy v6.8.1 #8432

Merged

Uh oh!

refactor: migrate the merge infographic component from emotion to chakraui #6374 #8246

refactor: migrate the merge infographic component from emotion to chakraui #6374 #8246

Uh oh!

Conversation

victorpatru commented Oct 13, 2022

Uh oh!

gatsby-cloud bot commented Oct 13, 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!

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

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!

victorpatru commented Oct 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

victorpatru commented Oct 25, 2022

Uh oh!

pettinarip left a comment

Choose a reason for hiding this comment

Uh oh!

gitpoap-bot bot commented Oct 29, 2022

Uh oh!

pettinarip commented Oct 29, 2022

Uh oh!

allcontributors bot commented Oct 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gatsby-cloud bot commented Oct 13, 2022 •

edited

Loading