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

feature: v2 #7

Merged
merged 18 commits into from
Nov 28, 2023
Merged

feature: v2 #7

merged 18 commits into from
Nov 28, 2023

Conversation

marlonmarcello
Copy link
Contributor

@marlonmarcello marlonmarcello commented Nov 22, 2023

Description

I was about to start work on the Modal component for the component library and noticed that this could use some love so one thing led to another and I ended up rewriting it and modernizing the setup as well.

If this works well, our other libraries can follow suit and start shipping types and CSS modules.

V2

Changes

The old design of the modal was honestly great but it had an achilles heel, it used an internal context to handle the transition and syncing that with state which caused:

  1. Actions to be tied to a context, adding custom components to close the modal for example was a bit annoying
  2. Animation and state were too tightly coupled and not exposed for further customization

To solve these I decoupled things and inverted the control from the component to the hook

  1. Now the modal component only cares about rendering
  2. The hook takes care of the logic and expose the variables in the shape that the modal expects so the developer don't have to worry
  3. It opened the door to use custom component for things like closing the modal and backdrop, so much so that we don't need to ship a close button anymore and we could honestly get away with shipping a backdrop component, it's just a positioned absolute div with a background color, too but decided to keep for convenience as that is something very common that almost never changes
  4. It's much easier to reason about animation now as the duration is directly exposed
  5. No need for context anymore, the hook takes care of it
  6. The logic for dealing with slugs is simpler, it's automatic, you don't need to set it

This change also makes issue #2 irrelevant. Customizing the component is just a matter os styling with css and the only trick is the transitionDuration which is I explained in the README updates.

New things

Modernized the library with:

  1. CSS modules
  2. Typescript

Screenshots

Screen.Recording.2023-11-21.at.7.36.22.PM.mov

Usage

Simple

No custom transition, no focus management

function MyModal() {
  const { isOpen, state, toggle } = useModal()

  return (
    <>
      <button onClick={toggle}>
        Open modal
      </button>

      {isOpen && (
        <Modal state={state}>
          <ModalBackdrop onClick={toggle} />
          <ModalContent>
            <button onClick={toggle}>
              Close
            </button>
            <p>Voluptate Lorem ut minim excepteur sit fugiat anim magna aliquip.</p>
          </ModalContent>
        </Modal>
      )}
    </>
  )
}

Complete

With custom transition, focus management and hash trigger

function MyModal() {
  const TRANSITION = 800
  const triggerButton = useRef(null)
  const { isOpen, state, toggle } = useModal({
    triggerRef: triggerButton,
    transitionDuration: TRANSITION,
    // modal will automatically open close when hash changes
    slug: "modal-with-slug",
  })

  // Custom transition must be set on the <Modal> component, how you do it, inline styles or as a className, is up to you
  const stylesVars = {
    "--transition-duration": `${TRANSITION}ms`,
  }

  return (
    <>
      {/* `triggerRef` allows the focus to shift to whatever triggered the modal, on close. */}
      <button ref={triggerButton} onClick={toggle}>
        Open the modal window!
      </button>

      {isOpen && (
        <Modal state={state} style={stylesVars}>
          <ModalBackdrop onClick={toggle} />
          <ModalContent
            className={classnames([
              // up to you how you want to handle the animation
              // you can also add separate classes for each state to customize the animation even further
              // you can also simply customize the parent <Modal> component
              // etc...
              'my-modal',
              (state === ModalStates.OPENING || state === ModalStates.OPEN) && 'my-modal-open',
            ])}
          >
            <button onClick={toggle}>
              Close
            </button>
            <p>Voluptate Lorem ut minim excepteur sit fugiat anim magna aliquip.</p>
          </ModalContent>
        </Modal>
      )}
    </>
  )
}

@marlonmarcello marlonmarcello self-assigned this Nov 22, 2023
@marlonmarcello marlonmarcello added the enhancement New feature or request label Nov 22, 2023
@marlonmarcello marlonmarcello changed the title wip: design v2 feature: v2 Nov 22, 2023
@marlonmarcello
Copy link
Contributor Author

I am thinking of changing slug to hash.

Copy link
Member

@andrewrubin andrewrubin left a comment

Choose a reason for hiding this comment

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

Great work @marlonmarcello , I dig the new setup. A few ideas and tweaks from me, below. One other thing which I cannot properly comment on — we should enforce stylelint/order on these CSS modules

Thanks, lmk if you want to chat about any of this!

src/lib/hooks/use-modal.ts Outdated Show resolved Hide resolved
src/lib/hooks/use-modal.ts Show resolved Hide resolved
src/main.module.css Outdated Show resolved Hide resolved
src/lib/hooks/use-modal.ts Outdated Show resolved Hide resolved
src/lib/hooks/use-modal.ts Outdated Show resolved Hide resolved
Copy link

changeset-bot bot commented Nov 24, 2023

🦋 Changeset detected

Latest commit: f533c1e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@wethegit/react-modal Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@marlonmarcello
Copy link
Contributor Author

All done now @andrewrubin

@andrewrubin
Copy link
Member

All changes looking good!

We should release a beta version to make sure that adding react and react-dom as dependencies doesn't break anything when this is used in-situ on a React project. I've run into issues many times before where the doubled-up react dependency breaks the build.

@marlonmarcello marlonmarcello marked this pull request as draft November 28, 2023 18:12
@marlonmarcello
Copy link
Contributor Author

Converting it to a Draft while we wait for hooks v2

@marlonmarcello marlonmarcello marked this pull request as ready for review November 28, 2023 22:20
@marlonmarcello marlonmarcello merged commit 4512c52 into main Nov 28, 2023
1 check passed
@marlonmarcello marlonmarcello deleted the feature/v2 branch November 28, 2023 22:29
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: add section on styling, available CSS vars, custom animations, etc.
2 participants