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

Breaking: Deprecate Reach-UI based modal and add Ariakit-based modal #686

Merged
merged 16 commits into from
Aug 30, 2022

Conversation

frankieyan
Copy link
Member

@frankieyan frankieyan commented Aug 18, 2022

Closes #679

Short description

This re-implements the modal with Ariakit. It tries to match the previous implementation as close as possible, both API and markup wise, so we don't leverage Ariakit's default modal backdrop.

Test plan

Test that all of the previous modal's functionality have been retained in the storybook demos.

Old: http://localhost:6006/?path=/docs/components-modal
New: http://localhost:6006/?path=/story/design-system-modal

  • The modal can be closed by clicking on the backdrop or pressing Esc
  • Right clicking on the backdrop doesn't close the modal
  • The modal isn't rendered in the DOM when it's closed
  • Other elements that are siblings to the modal have aria-hidden set when the modal is open
  • Content outside the modal isn't scrollable when it's open
  • The modal's contents should be scrollable
  • The width/size props should be respected
  • Go to "Canvas" mode for the "Modal with standard actions footer" demo and open the modal. In the overflow menu, click on each option and check that they're logged to the "Actions" tab below
  • In the "Autofocus" demo, check that the "Address" text field is focused
  • In the "Stacking modals" demo, check that you can open a modal on top of another, and switch its props independently via the form within the modal

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Updated all static build artifacts (npm run build-all)

Versioning

Major

@frankieyan frankieyan requested review from a team and scottlovegrove and removed request for a team August 18, 2022 17:36
@frankieyan frankieyan force-pushed the frankie/ariakit-modal branch 2 times, most recently from 6bca836 to 4aaff2c Compare August 19, 2022 06:11
Copy link
Contributor

@scottlovegrove scottlovegrove left a comment

Choose a reason for hiding this comment

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

✅ All works as expected

@frankieyan frankieyan force-pushed the frankie/ariakit-modal branch from 2d6b358 to d903753 Compare August 25, 2022 13:32
@frankieyan
Copy link
Member Author

frankieyan commented Aug 26, 2022

@scottlovegrove I added a couple more fixes after testing it in Todoist:

  • Added a storybook demo to show rendering a modal on top of another
  • Added a Menu component to the first demo to show menu items can be selected, and that they can be traversed with the keyboard

Could you please have a quick look at the last few commits and give them a spin in Storybook?

Copy link
Contributor

@gnapse gnapse left a comment

Choose a reason for hiding this comment

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

Preliminary review, before I get into the new steps in the test plan:

  1. The header title is not aligned to the close button:

    CleanShot 2022-08-29 at 11 06 43@2x

    This only happens in the first story, the one titled "Modal with standard actions footer". So it's most probably a problem with the story, and not with the modal component itself. It probably has to do with how you put this custom menu button in the header. I left a comment in code on what I think may fix this.

    More info

    I realized while switching between two tabs: one with the new modal, and the other one with the old modal.

    CleanShot 2022-08-29 at 11 09 05

  2. In the "Stacking modal" story, I suggest changing the nested modal to have a different size than the parent modal initially, so that we can see how a modal is floating in top of the other one. That is, a different height and width setting.

<Modal aria-label="Modal with standard actions footer">
<ModalHeader>
<Heading level="1">Modal with standard actions footer</Heading>
<Columns>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this fixes the misaligned modal header:

Suggested change
<Columns>
<Columns alignY="center">

Although, IMO, this extra button should be put in the header button slot, instead of putting it in the title slot. Something along these lines:

<ModalHeader
  button={
    <Columns space="small">
      <Column><CustomButton /></Column>
      <Column><ModalCloseButton /></Column>
    </Columns>
  }
>
  <Heading level="1">Modal with standard actions footer</Heading>
</ModalHeader>

@frankieyan frankieyan force-pushed the frankie/ariakit-modal branch from 7368b86 to e335b66 Compare August 30, 2022 08:58
@frankieyan frankieyan merged commit c73fe63 into main Aug 30, 2022
@frankieyan frankieyan deleted the frankie/ariakit-modal branch August 30, 2022 11:15
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.

Help update @reach/dialog to be compatible with React 18, or migrate to another compatible backing component
3 participants