Skip to content
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.

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

feat(common-ui): Add Modal Confirm and Provider #2749

Merged
merged 4 commits into from
Apr 25, 2022

Conversation

victorgcramos
Copy link
Member

This Diff adds ModalProvider and ModalConfirm components
on common-ui package.

Preview

Screen Shot 2022-04-14 at 12 50 15 PM

Usage

You can open and close modals using the useModal hook:

import { ModalConfirm, ModalProvider, useModal } from "../components";

function ModalButton() {
  const [open] = useModal();
  return (
    <Button
      onClick={() => open(ModalConfirm)}
    >
      Open Modal
    </Button>
  );
}

function App() {
  return (
    <ModalProvider>
      <ModalButton />
    </ModalProvider>
  );
}

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

The way you are using the ModalConfirm component makes the React.memo irrelevant. In fact, it's making performance a little bit worse due to the shallow comparisons between new and old props every time. See:

open(ModalConfirm, {
          onSubmit: () => {
            console.log("confirmed");
          },
        })

onSubmit will never be the same function. It will always create a new function in this case. This happens always that you create a function inline or inside the component (and the component is unmounted and mounted again).

I'd say you can remove React.memo HOC here.

Everything else looks good.

Copy link
Member

@tiagoalvesdulce tiagoalvesdulce left a comment

Choose a reason for hiding this comment

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

LGTM

@tiagoalvesdulce tiagoalvesdulce merged commit 78607c3 into decred:master Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants