Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet #507

0xChijioke · 2023-08-26T10:29:07Z

Description

Hello! This pull request introduces a reusable TriggerModal component and integrates it into the RainbowKitCustomConnectButton and Faucet components. This modification aims to improve developer/user experience and code reuseability.

video1299019585.mp4

Additional Information

I have read the contributing docs (if this is your first contribution)
This is not a duplicate of any existing pull request

Your ENS/address: chijoke.eth

rin-st · 2023-08-26T15:45:56Z

Hi @0xChijoke , thank you for the pr!

I believe we should ask designers for big changes on the example-ui page.

So in this pr let's polish new modal component and use it for components where we have modals already - RainbowKitCustomConnectButton and Faucet.

We can add modal for example-ui page later if needed

rin-st · 2023-08-26T15:55:59Z

packages/nextjs/components/Modal.tsx

+ */
+type ModalProps = {
+  trigger: JSX.Element;
+  modalTitle: string;


We need to use that prop for htmlsFor and id attributes but not for title. So I think name or `id' is better.

Title should be in different place, because it can be empty or long with spaces and can break labels/ids.

rin-st · 2023-08-26T15:56:38Z

packages/nextjs/components/Modal.tsx

+type ModalProps = {
+  trigger: JSX.Element;
+  modalTitle: string;
+  modalContent: JSX.Element;


what about just using children instead of this prop

rin-st · 2023-08-26T16:00:00Z

packages/nextjs/components/Modal.tsx

+  };
+};
+
+const Modal: React.FC<ModalProps> = ({ trigger, modalTitle, modalContent, action }) => {


Basically, it's not only modal, but also trigger. So maybe it should be named like TriggerWithModal? Check prop types name too if you'll change component name.

Sounds good.

Does TriggerModal work?

rin-st · 2023-08-26T16:03:17Z

packages/nextjs/components/Modal.tsx

+  trigger: JSX.Element;
+  modalTitle: string;
+  modalContent: JSX.Element;
+  action?: {


It can be two or more buttons, for example "cancel" and "save". So probably we need to include this in content too

Exactly, the actions can be included in the modal content.

In the exampe-ui the send is in the modalContent.

The action is optional cause I had a specific use for passing it in.

It can also be removed if there is no need to leave the option there.

rin-st · 2023-08-26T16:09:13Z

packages/nextjs/components/example-ui/ContractInteraction.tsx

@@ -19,6 +20,45 @@ export const ContractInteraction = () => {
    },
  });

+  // Define the modal trigger button


let's change this file when we'll have designs
#466

0xChijioke · 2023-08-26T18:07:31Z

@rin-st Let me know what you think about the current state of the component.

Should the optional action be removed because that can come with the children?

Would I need to create another PR to leave other files in there original state?

rin-st · 2023-08-27T05:48:41Z

Should the optional action be removed because that can come with the children?

Yes, we don't need it for now

Would I need to create another PR to leave other files in there original state?

No, you can rename this pr and remove changes of other files. We can take those changes if needed from commits history.

When you complete the component, please change custom modals in RainbowKitCustomConnectButton and Faucet

Thanks!

rin-st · 2023-08-27T05:50:00Z

packages/nextjs/components/TriggerModal.tsx

+  };
+};
+
+const Modal: React.FC<TriggerModalProps> = ({ trigger, name, children, action }) => {


rin-st · 2023-08-27T05:50:21Z

packages/nextjs/components/TriggerModal.tsx

@@ -0,0 +1,42 @@
+/**
+ * Modal component: A reusable modal component.


redundant comment

rin-st · 2023-08-27T05:50:34Z

packages/nextjs/components/TriggerModal.tsx

+  trigger: JSX.Element;
+  name: string;
+  children: JSX.Element;
+  action?: {


remove for now

…itConnectButton and Faucet components.

0xChijioke · 2023-08-28T13:09:48Z

Hello @rin-st,

While refactoring the RainbowKitConnectButton component, I encountered a scenario where rendering the trigger separately from the modal body would be beneficial.

To address this, I've split the component into two parts and introduced a new folder called TriggerWithModal. Within this folder, you'll find the Trigger and Modal components, each serving their specific roles and enhancing modularity.

I've already utilized this structure in both the RainbowKitConnectButton and Faucet components. Your feedback on this approach and any potential adjustments would be greatly appreciated.

Thanks!

…into example-ui

rin-st · 2023-08-29T04:08:25Z

Hi @0xChijoke , good job!

I don't like two-component nature of TriggerWithModal but it seems in this case it's a good solution.

I've tested your branch and found that modal doesn't close on click outside, as it was before. Fix it, please

rin-st · 2023-08-29T04:09:55Z

packages/nextjs/components/scaffold-eth/Faucet.tsx

@@ -82,49 +84,42 @@ export const Faucet = () => {

  return (
    <div>
-      <label htmlFor="faucet-modal" className="btn btn-primary btn-sm px-2 rounded-full font-normal normal-case">
+      <Trigger id="faucet-modal" className="btn btn-primary btn-sm px-2 rounded-full font-normal normal-case">


and it's better to move id to constant outside of component and use it in both trigger and modal

rin-st · 2023-08-29T04:10:09Z

packages/nextjs/components/scaffold-eth/RainbowKitCustomConnectButton.tsx

@@ -134,10 +136,10 @@ export const RainbowKitCustomConnectButton = () => {
                        )}
                      </li>
                      <li>
-                        <label htmlFor="qrcode-modal" className="btn-sm !rounded-xl flex gap-3 py-3">
+                        <Trigger id="qrcode-modal" className="btn-sm !rounded-xl flex gap-3 py-3">


I've incorporated the changes you mentioned.

I've also added an option for the modal to be closed on outside clicks(true by default), considering its reusability.

This way, the modal can be used in various scenarios, allowing builders to choose the preferred behaviour.

Your patience and guidance are much appreciated. Looking forward to your feedback!

…odal behavior

technophile-04 · 2023-08-29T10:52:27Z

Hey guys thanks, but I don't see much advantages of adding this component :

With DaisyUI v3 modal:

<button onClick={() => window.faucet_modal.showModal()} className="...">
   ...
  <span>Faucet</span>
</button>

<dialog id={"faucet_modal"} className="modal">
  <form method="dialog" className="modal-box">
    ...
  </form>
</dialog>

With this component :

<Trigger id={FAUCET_MODAL_ID} className="...">
  ...
  <span>Faucet</span>
</Trigger>

<Modal id={FAUCET_MODAL_ID}>
  ...
</Modal>

Since we are using a component library (daisyUI v3) I think it's not worth creating our own micro components unless we have good reason to or there is something that is not present / not working in daisyUI

Also, I think by creating this component we are just abstracting 1-2 lines of code, I think its we let user use daisy UI and learn/customize as they want

What do you guys think?

0xChijioke · 2023-08-29T11:51:13Z

Hey guys thanks, but I don't see much advantages of adding this component :

With DaisyUI v3 modal:
<button onClick={() => window.faucet_modal.showModal()} className="...">
   ...
  <span>Faucet</span>
</button>

<dialog id={"faucet_modal"} className="modal">
  <form method="dialog" className="modal-box">
    ...
  </form>
</dialog>
With this component :
<Trigger id={FAUCET_MODAL_ID} className="...">
  ...
  <span>Faucet</span>
</Trigger>

<Modal id={FAUCET_MODAL_ID}>
  ...
</Modal>
Since we are using a component library (daisyUI v3) I think it's not worth creating our own micro components unless we have good reason to or there is something that is not present / not working in daisyUI

Also, I think by creating this component we are just abstracting 1-2 lines of code, I think its we let user use daisy UI and learn/customize as they want

What do you guys think?

I share your perspective. While the initial intention was to simplify modal usage, it seems that introducing custom components might actually lead to unnecessary complexity.

technophile-04 · 2023-08-29T13:27:23Z

I share your perspective. While the initial intention was to simplify modal usage, it seems that introducing custom components might actually lead to unnecessary complexity.

Yeah, but tysm @0xChijoke for questing on this !! Also, Chijoke would really appreciate creating an issue or discussion before creating a new feature PR so that we can iterate and brainstorm on it if it is really required / others can put suggestions/improvements on it too 🙌

Really feel bad closing this PR but appreciate it !! Also, tysm to Rinat for reviewing this 🙌

rin-st · 2023-08-29T16:25:26Z

I think by creating this component we are just abstracting 1-2 lines of code, I think its we let user use daisy UI and learn/customize as they want

hmm, I'm not very agree, because

why to force users to learn something if we can avoid it? yes it's a small thing, but it can take let's say 5-10 mins to configure it instead of two imports. Users can forget to add clickoutside and so on
we have at least two modals for now, and if we want for example add shadow we need to do it in two places instead of one

Yes, if we want to add modal, we need to add most of the options from daisyui, and also add other components like Button and other simple things. It simplifies development but we need to think if we really need it.

Thanks @0xChijoke for your efforts and @technophile-04 for review!

rin-st reviewed Aug 26, 2023

View reviewed changes

rin-st reviewed Aug 27, 2023

View reviewed changes

0xChijioke changed the title ~~enhancement(UI): Added a reuseable modal component and used it for the set greeting input.~~ Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet Aug 27, 2023

0xChijioke closed this Aug 28, 2023

0xChijioke force-pushed the example-ui branch from 363688a to 2e15c39 Compare August 28, 2023 12:37

update: created and used Trigger and Modal component and for RainbowK…

3f1d4cb

…itConnectButton and Faucet components.

0xChijioke reopened this Aug 28, 2023

Merge branch 'scaffold-eth:main' into example-ui

bb8e993

0xChijioke added 2 commits August 28, 2023 14:49

update: add optional classNames for modal styling.

8acdae4

Merge branch 'example-ui' of https://github.com/ctteck/scaffold-eth-2 …

b956740

…into example-ui

rin-st reviewed Aug 29, 2023

View reviewed changes

0xChijioke and others added 2 commits August 29, 2023 11:01

refactor: Use constant for modal ID and introduce optional prop for m…

4e0119b

…odal behavior

Merge branch 'scaffold-eth:main' into example-ui

5717583

technophile-04 closed this Aug 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet #507

Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet #507

0xChijioke commented Aug 26, 2023 •

edited

Loading

rin-st commented Aug 26, 2023

rin-st Aug 26, 2023

rin-st Aug 26, 2023

rin-st Aug 26, 2023

0xChijioke Aug 26, 2023

rin-st Aug 26, 2023

0xChijioke Aug 26, 2023

rin-st Aug 26, 2023

0xChijioke commented Aug 26, 2023

rin-st commented Aug 27, 2023

rin-st Aug 27, 2023

rin-st Aug 27, 2023

rin-st Aug 27, 2023

0xChijioke commented Aug 28, 2023 •

edited

Loading

rin-st commented Aug 29, 2023

rin-st Aug 29, 2023

rin-st Aug 29, 2023

0xChijioke Aug 29, 2023

technophile-04 commented Aug 29, 2023

0xChijioke commented Aug 29, 2023

technophile-04 commented Aug 29, 2023

rin-st commented Aug 29, 2023

		@@ -0,0 +1,42 @@
		/**
		* Modal component: A reusable modal component.

Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet #507

Reusable TriggerModal component in RainbowKitCustomConnectButton and Faucet #507

Conversation

0xChijioke commented Aug 26, 2023 • edited Loading

Description

Additional Information

rin-st commented Aug 26, 2023

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

0xChijioke commented Aug 26, 2023

rin-st commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xChijioke commented Aug 28, 2023 • edited Loading

rin-st commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technophile-04 commented Aug 29, 2023

0xChijioke commented Aug 29, 2023

technophile-04 commented Aug 29, 2023

rin-st commented Aug 29, 2023

0xChijioke commented Aug 26, 2023 •

edited

Loading

0xChijioke commented Aug 28, 2023 •

edited

Loading