[ShadCN] Migrate Modal component to ShadCN/Tailwind #14163

TylerAPfledderer · 2024-10-14T00:05:31Z

Description

Migrate Modal component to ShadCN to build off of the Dialog component.

Replaces only the instances of the Modal currently in prod.

netlify · 2024-10-14T00:05:48Z

✅ Deploy Preview for ethereumorg ready!

Name	Link
🔨 Latest commit	`23bab1d`
🔍 Latest deploy log	https://app.netlify.com/sites/ethereumorg/deploys/6770a9b44346930008454745
😎 Deploy Preview	https://deploy-preview-14163--ethereumorg.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	7 paths audited Performance: 52 (🔴 down 4 from production) Accessibility: 92 (no change from production) Best Practices: 89 (🔴 down 9 from production) SEO: 98 (no change from production) PWA: 59 (🟢 up 30 from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

TylerAPfledderer · 2024-10-17T14:52:57Z

Currently trying to leverage tailwind-variants to come up with a good approach to make the Modal styling a priority, and the other styles as variants. particularly with the placement of the close button.

pettinarip · 2024-10-17T17:26:29Z

Currently trying to leverage tailwind-variants to come up with a good approach to make the Modal styling a priority, and the other styles as variants. particularly with the placement of the close button.

Great! thanks for the update @TylerAPfledderer. I'll have a closer look tomorrow.

TylerAPfledderer · 2024-10-17T17:57:22Z

@pettinarip I've noticed an error in my approach that I'll have corrected soon.

I should allow disclosure control to come purely from a parent of the modal, and consider including the dialog trigger in the renders if they aren't being used already.

pettinarip · 2024-10-18T12:13:10Z

src/components/ui/dialog-modal.tsx

+      <Center className="absolute end-0 top-0" asChild>
+        <DialogPrimitive.Close className={close()}>
+          <MdClose />
+          <span className="sr-only">Close</span>
+        </DialogPrimitive.Close>
+      </Center>


To give full control, perhaps we could avoid forcing this and declare it as part of the DialogClose button.

Then, we could be more opinionated in the Modal structure and declare it as part of the header.

<DialogHeader> <DialogTitle>{title}</DialogTitle> <Center className="absolute end-0 top-0" asChild> <DialogClose /> </Center> </DialogHeader>

We could use our Button to wrap the close button to inherit the styles, right?

<DialogPrimitive.Close asChild> <Button ...>... </DialogPrimitive.Close>

To give full control, perhaps we could avoid forcing this and declare it as part of the DialogClose button.

Can you clarify what you mean here? Are you referring specifically to DialogTitle?

Hmmm I'm not sure either way about the use of Button component. Best would be

<Button variant="ghost" isSecondary size="sm">

though here the size might be too small, but size="md" is too tall compared to the figma file, and you would have to force overrides in the size.

pettinarip · 2024-10-18T12:16:58Z

src/components/ui/dialog-modal.tsx

+  contentProps?: DialogContentProps
+}
+
+const Modal = ({


Nice. So, this export is to be backwards compatible with the previous instances, right?

Anyway, I think this abstraction makes sense to avoid repeating this code and have more consistency.

Was thinking if this was the correct place to put it or in src/components/Modal.tsx.

Yea. Prop naming has changed slightly.

I currently have it here just to keep this isolated from the existing Modal file along with having all of this isolated from the existing dialog file while I try to figure out how all of it should be merged together.

pettinarip · 2024-10-18T12:18:46Z

src/components/ui/dialog-modal.tsx

+const dialogVariant = tv({
+  slots: {
+    content:
+      "data-[state=open]:animate-contentShow w-full fixed left-1/2 top-1/2 grid -translate-x-1/2 -translate-y-1/2 gap-4 rounded-md bg-white p-8 shadow-[hsl(206_22%_7%_/_35%)_0px_10px_38px_-10px,_hsl(206_22%_7%_/_20%)_0px_10px_20px_-15px] focus:outline-none",


contentShow? is that a built-in animation in TW?

That is a utility that came from Radix as I was copy/pasting some default code. I'll review this.

TylerAPfledderer · 2024-10-19T03:05:48Z

@pettinarip Not sure what should be done here using this new modal for the wallet simulators. Per how logic works in radix between DialogTrigger and the dialog itself, it would require rebuilding the code structure of the simulator content to retain this internal logic and avoid the need for a controlled state from the parent (but maintain the path querying to render the modal content).

I could try and keep the current way the simulator modals are triggered, but could be become async with the intended approach in using this modal in other areas.

Would at least keep this temporary dialog-modal file for now, while doing incremental transitioning, and ignore the simulator area for now.

At the time of this comment, I have not viewed other renders that are currently using the existing Modal.

TylerAPfledderer · 2024-10-30T14:05:50Z

Update: Not sure what I should do here.

This component seems to require in its pattern to have the DialogTrigger in order for the open/close logic to function correctly. In this case, the modals related to the simulator, quiz list, CodeModal, and a couple of other locations would need to be updated to account for this. That should be a separate PR.

I could for now ignore this and maintain the controlled logic currently present. Not sure if there will be unintended results.

pettinarip · 2024-11-08T18:05:59Z

Update: Not sure what I should do here.

This component seems to require in its pattern to have the DialogTrigger in order for the open/close logic to function correctly. In this case, the modals related to the simulator, quiz list, CodeModal, and a couple of other locations would need to be updated to account for this. That should be a separate PR.

I could for now ignore this and maintain the controlled logic currently present. Not sure if there will be unintended results.

Hey @TylerAPfledderer not sure if I follow you 100% on all you described but you can have a controlled Dialog without a DialogTrigger, its optional. At least in my tests, I didn't get any warnings or errors by doing that and having a fully controlled dialog.

So, for the Simulator, this is how I see it

// current layout
<grid>
    <buttons>
    <simulatormodal>
        <template ...>
    </simulatormodal>
</grid>

// new layout
<Dialog open={..}>
    <grid>
        <buttons />

        <DialogContent>
            <template ...>
        </DialogContent>
    </grid>
</Dialog>

And if you were talking about keeping the same API as the SimulatorModal, then yes, I think we will have to adapt in each case. And I agree with you that this "adaptation work" might be better addressed in another PR.

TylerAPfledderer · 2024-11-11T04:50:45Z

@pettinarip noting that I do not have any objection to your response, and currently addressing the areas using the current custom Modal and replacing it. Currently not experiencing issues.

I would still consider revisiting this in a future PR in improving disclosure.

TylerAPfledderer · 2024-11-13T05:53:38Z

@pettinarip I believe I am at a point for a proper review.

I'm thinking that it should be left as a separate dialog-modal.tsx file for now. The goal would be to unify the usage of dialog right? I only replaced all the current instances of the custom Modal and I see that Dialog and Chakra's Modal are being imported directly in other places.

And aside from that, I think there still might be a benefit to using DialogTrigger instead of outside controlled state, and be able to run trackCustomEvent via onOpenChange prop, along with anything else that relies on the open/close state. This way, additional disclosure logic can be removed.

pettinarip · 2024-11-27T18:28:20Z

@pettinarip I believe I am at a point for a proper review.

I'm thinking that it should be left as a separate dialog-modal.tsx file for now. The goal would be to unify the usage of dialog right? I only replaced all the current instances of the custom Modal and I see that Dialog and Chakra's Modal are being imported directly in other places.

Cool, got it.

I think we will need to leave both, the dialog and dialog-modal. dialog is useful for the command component (currently used in the lang picker).

The only chakra modal still pending would be the CodeModal, right? wondering if you are going to tackle that one in this PR.

And aside from that, I think there still might be a benefit to using DialogTrigger instead of outside controlled state, and be able to run trackCustomEvent via onOpenChange prop, along with anything else that relies on the open/close state. This way, additional disclosure logic can be removed.

Sounds good, are you planning on doing that in a different PR?

pettinarip

@TylerAPfledderer gj, this looks solid. I'll see if I can get a review from designers but looks ready to me.

TylerAPfledderer · 2024-11-27T18:59:02Z

@pettinarip

The only chakra modal still pending would be the CodeModal, right? wondering if you are going to tackle that one in this PR.

I believe I didn't touch it because it was not connected to the original custom modal. (The modal is a direct import from Chakra) For how old this PR is now, I would look at that in a separate PR to not let this one linger for any longer than needed.

And I would like to address the use of DialogTrigger in a separate PR as well.

src/components/Simulator/index.tsx

pettinarip

@TylerAPfledderer on desktop it looks like everything is working fine but in mobile there are a few things left.

You can't scroll if the content it too large. In some cases, even the close button is not visible.
We had a margin before wrapping the modal container and you could see a bit of the dark overlay, do you think it is possible to replicate?

This happens in the quizzes page or in the simulator
PR

Prod

Minor request: could it be possible to make the close button icon a bit bigger? as we have in prod? is 32px in prod, in this PR is 16px.

TylerAPfledderer · 2024-12-13T20:05:40Z

Hey @pettinarip! My apologies in delay with in responding to your latest review.

I will be taking a look at these two items right now. I do not expect any issue, but will notify you immediately if I run into a problem I cannot find a quick solution for.

I will also review the design in Figma. I'll let you know if I happen to see any discrepancy there vs. prod

TylerAPfledderer

@pettinarip I made updates with comments provided. The comment regarding scroll needs a recommendation, as this PR is not complete at this point.

TylerAPfledderer · 2024-12-13T21:34:38Z

src/components/ui/dialog-modal.tsx

+      /**
+       * `size-[calc(-2rem_+_100%)]` provides 16px x- and y-spacing
+       * around the container instead of using `margin` to
+       * avoid positioning side-effects.
+       * (The container is fixed-positioned)
+       *
+       * Credit: Chakra v2 takes a similar approach for modal
+       */


Comment regarding using of calc with height and width instead of margin to have spacing around the container when the viewport gets too small.

TylerAPfledderer · 2024-12-13T21:43:06Z

src/components/ui/dialog-modal.tsx

+       *
+       * Credit: Chakra v2 takes a similar approach for modal
+       */
+      "data-[state=open]:animate-contentShow size-[calc(-2rem_+_100%)] fixed left-1/2 top-1/2 grid -translate-x-1/2 -translate-y-1/2 gap-4 rounded-md bg-background p-8 shadow-[hsl(206_22%_7%_/_35%)_0px_10px_38px_-10px,_hsl(206_22%_7%_/_20%)_0px_10px_20px_-15px] focus:outline-none z-modal  overflow-auto",


Note that I add overflow-auto here in conjunction to setting height (see above comment) to allow for scroll when the container has a lot of content.

I'm torn here. When applying this quick fix, this creates space on the top and bottom around the container to prevent the container from hitting the edge of the screen. Prod does not have bottom spacing because the container is allowed to expand below the fold, and you are actually scrolling the rest of the container into view and not only the content.

The prod version allows for scrolling from below the fold into view because there is a wrapper around the container which is not present in the current usage of the radix component. I would have to create this wrapper manually and wrap DialogPrimitive.Content. This would also need to update where the styling is assigned to here.

I prefer the UX of prod, but wanted to show you what my quick fix looks like. If you are in agreement to maintain what is in prod, should I address it here or in a separate PR? I do not see this as a big fix, but I don't want to bloat or extend the life of this PR any more than necessary.

Update: I saw in Chromatic that my approach resulted in a side-effect of expanding the height without a max. (you'll see this when you review) This is correct behavior, but should not be employed unless there is a max-height that should be defined from the design. I think this further justifies addressing how to approach adding the content wrapper I described above.

Looking at this, I think this effect will be easier if we make the Overlay wrap the Content (right now it is a sibling which make it hard to place it).

Following this guide, we could do

// this const DialogContent = () => { return ( <DialogPortal> <DialogOverlay> <DialogPrimitive.Content> {children} </DialogPrimitive.Content> </DialogOverlay> </DialogPortal> ) }) // instead of <DialogPortal> <DialogOverlay /> <DialogPrimitive.Content /> </DialogPortal>

and then,

overlay => fixed inset-0 overflow-y-auto p-4 content => free, no positioning

Of course, this would be only for mobile screens, the previous styles should still apply to desktop resolutions.

Ah I missed that. No problem!

TylerAPfledderer · 2024-12-13T21:45:01Z

src/components/ui/dialog-modal.tsx

+      {children}
+      <Center className={close()} asChild>
+        <DialogPrimitive.Close>
+          <MdClose size="20" />


This close button is different from prod, because the prod version comes from Chakra, which has it's own close icon, and different than MdClose. Therefore, I chose to use react-icons size prop to set a different value to match prod.

Looks good!

pettinarip · 2024-12-23T18:51:42Z

Hey @pettinarip! My apologies in delay with in responding to your latest review.

I will be taking a look at these two items right now. I do not expect any issue, but will notify you immediately if I run into a problem I cannot find a quick solution for.

I will also review the design in Figma. I'll let you know if I happen to see any discrepancy there vs. prod

No worries. This is a pretty heavy task. I left a comment about the mobile version. LMK if you agree on that. I think we are very close on this one.

TylerAPfledderer · 2024-12-27T01:33:21Z

@pettinarip alright, that wrapping approach is better. Looks like Overlay needed to use display: grid to position the content center and allow for the overflow to be handled correctly with scroll.

Also made a comment in Chromatic regarding padding for the quiz modal (unless I ended up addressing it before you see it)

TylerAPfledderer · 2024-12-29T01:52:24Z

@pettinarip Applied a missing style. Need the Quiz Modal snapshot reviewed.

pettinarip

Awesome @TylerAPfledderer! gj

feat: create ShadCN modal with temporary dialog

0003319

TylerAPfledderer force-pushed the feat/shadcn-modal branch from f5592cd to 0003319 Compare October 14, 2024 00:09

TylerAPfledderer added 2 commits October 15, 2024 23:26

chore(ui/Modal): rename "Full" story to "Xl"

85ec946

refactor(ui/dialog-modal): create provider for variant styles

3c246ef

TylerAPfledderer force-pushed the feat/shadcn-modal branch from ab4905e to 3c246ef Compare October 17, 2024 03:44

pettinarip reviewed Oct 18, 2024

View reviewed changes

pettinarip added the hacktoberfest Track hacktoberfest activity label Oct 24, 2024

TylerAPfledderer added 2 commits November 10, 2024 22:06

refactor(QuizzesModal): migrate to dialog-modal

8b9bc21

refactor(Simulator): migrate to dialog-modal

27fd3f8

TylerAPfledderer added 5 commits November 11, 2024 10:37

refactor(FileContributors): migrate to dialog-modal

a8405f2

fix(dialog-modal): set dynamic bg color to modal content

66a0516

refactor(pages/developers/tutorials): migrate to dialog-modal

902f28c

Merge remote-tracking branch 'upstream/dev' into feat/shadcn-modal

081c879

refactor: remove old Modal and replace stories

9e26eae

TylerAPfledderer marked this pull request as ready for review November 13, 2024 05:23

TylerAPfledderer requested review from wackerow, corwintines and minimalsm as code owners November 13, 2024 05:23

refactor(dialog-modal): remove internal disclosure from Modal

be315b2

pettinarip approved these changes Nov 27, 2024

View reviewed changes

Merge remote-tracking branch 'upstream/dev' into feat/shadcn-modal

c159b9b

TylerAPfledderer commented Dec 2, 2024

View reviewed changes

src/components/Simulator/index.tsx Outdated Show resolved Hide resolved

chore(Simulator): remove console.log

aecb15c

pettinarip requested changes Dec 4, 2024

View reviewed changes

TylerAPfledderer added 3 commits December 13, 2024 15:38

fix(dialog-modal): set default left/right space around container

2dd4937

fix(dialog-modal): allow vertical overflow scrolling

2f5d51d

fix(dialog-model): increase close button icon size

a5fde9c

TylerAPfledderer commented Dec 13, 2024

View reviewed changes

TylerAPfledderer added 2 commits December 26, 2024 20:22

fix(dialog-modal): wrap overlay component around content

f4aa42e

Merge remote-tracking branch 'upstream/dev' into feat/shadcn-modal

4e43cfb

TylerAPfledderer added 2 commits December 28, 2024 20:44

fix(dialog-modal): set full width to content

d050268

chore(dialog-modal): remove comment above content style

23bab1d

pettinarip approved these changes Dec 30, 2024

View reviewed changes

pettinarip merged commit eda686f into ethereum:dev Dec 30, 2024
10 checks passed

TylerAPfledderer deleted the feat/shadcn-modal branch December 30, 2024 13:27

This was referenced Jan 2, 2025

RC v9.4.0 #14594

Merged

Deploy v9.4.0 #14596

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ShadCN] Migrate Modal component to ShadCN/Tailwind #14163

[ShadCN] Migrate Modal component to ShadCN/Tailwind #14163

TylerAPfledderer commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading

TylerAPfledderer commented Oct 17, 2024

pettinarip commented Oct 17, 2024

TylerAPfledderer commented Oct 17, 2024 •

edited

Loading

pettinarip Oct 18, 2024

pettinarip Oct 18, 2024

TylerAPfledderer Oct 19, 2024

pettinarip Oct 18, 2024

pettinarip Oct 18, 2024

TylerAPfledderer Oct 19, 2024

pettinarip Oct 18, 2024

TylerAPfledderer Oct 19, 2024

TylerAPfledderer commented Oct 19, 2024 •

edited

Loading

TylerAPfledderer commented Oct 30, 2024

pettinarip commented Nov 8, 2024

TylerAPfledderer commented Nov 11, 2024

TylerAPfledderer commented Nov 13, 2024 •

edited

Loading

pettinarip commented Nov 27, 2024

pettinarip left a comment

TylerAPfledderer commented Nov 27, 2024 •

edited

Loading

pettinarip left a comment

TylerAPfledderer commented Dec 13, 2024 •

edited

Loading

TylerAPfledderer left a comment •

edited

Loading

TylerAPfledderer Dec 13, 2024

TylerAPfledderer Dec 13, 2024

pettinarip Dec 23, 2024

TylerAPfledderer Dec 27, 2024

TylerAPfledderer Dec 13, 2024

pettinarip Dec 23, 2024

pettinarip commented Dec 23, 2024

TylerAPfledderer commented Dec 27, 2024

TylerAPfledderer commented Dec 29, 2024

pettinarip left a comment

[ShadCN] Migrate Modal component to ShadCN/Tailwind #14163

[ShadCN] Migrate Modal component to ShadCN/Tailwind #14163

Conversation

TylerAPfledderer commented Oct 14, 2024 • edited Loading

Description

netlify bot commented Oct 14, 2024 • edited Loading

✅ Deploy Preview for ethereumorg ready!

TylerAPfledderer commented Oct 17, 2024

pettinarip commented Oct 17, 2024

TylerAPfledderer commented Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TylerAPfledderer commented Oct 19, 2024 • edited Loading

TylerAPfledderer commented Oct 30, 2024

pettinarip commented Nov 8, 2024

TylerAPfledderer commented Nov 11, 2024

TylerAPfledderer commented Nov 13, 2024 • edited Loading

pettinarip commented Nov 27, 2024

pettinarip left a comment

Choose a reason for hiding this comment

TylerAPfledderer commented Nov 27, 2024 • edited Loading

pettinarip left a comment

Choose a reason for hiding this comment

TylerAPfledderer commented Dec 13, 2024 • edited Loading

TylerAPfledderer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettinarip commented Dec 23, 2024

TylerAPfledderer commented Dec 27, 2024

TylerAPfledderer commented Dec 29, 2024

pettinarip left a comment

Choose a reason for hiding this comment

TylerAPfledderer commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading

TylerAPfledderer commented Oct 17, 2024 •

edited

Loading

TylerAPfledderer commented Oct 19, 2024 •

edited

Loading

TylerAPfledderer commented Nov 13, 2024 •

edited

Loading

TylerAPfledderer commented Nov 27, 2024 •

edited

Loading

TylerAPfledderer commented Dec 13, 2024 •

edited

Loading

TylerAPfledderer left a comment •

edited

Loading