Skip to content

Conversation

@gnapse
Copy link
Contributor

@gnapse gnapse commented Dec 29, 2022

Short description

This is work extracted off of #730, that I deemed useful regardless of whether we adopt Chromatic or not. It is mostly about improvements I had to make to the modal stories to take advantage of Chromatic visual regression tests on modals, but these improvements add value on their own without adopting Chromatic.

Here's a list of the relevant changes:

  1. Rewrote modal stories into two modules:
    • MDX for text docs (mostly documenting component APIs and props)
    • .tsx for modal example stories that actually render the modal component
  2. Introduce a new portalElement prop on the modal, allowing consumers to customize where in the DOM they want their portal element to be mounted.
  3. Added storybooks interaction testing to modal stories.
    • This now makes it so that modals automatically open in modal stories.
    • This opens the door to use storybooks as a place where we run in-browser component tests.
  4. Various other small improvements to stories in other components.

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

A new minor version change, due to the new feature introduced to the modal component.

@gnapse gnapse requested a review from a team December 29, 2022 14:12
@gnapse gnapse self-assigned this Dec 29, 2022
@gnapse gnapse requested review from rfgamaral and removed request for a team December 29, 2022 14:12
"name": "@doist/reactist",
"description": "Open source React components by Doist",
"author": "Henning Muszynski <henning@doist.com> (http://doist.com)",
"author": {
Copy link
Member

Choose a reason for hiding this comment

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

💭 Nothing against Henning 😅, but doesn't it make more sense to remove this?

Copy link
Member

@rfgamaral rfgamaral left a comment

Choose a reason for hiding this comment

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

Looks good to me 🚀

@gnapse gnapse force-pushed the ernesto/modal-stories branch from d2ae781 to e169144 Compare January 3, 2023 12:06
@gnapse gnapse enabled auto-merge (squash) January 3, 2023 12:39
@gnapse gnapse merged commit 412de7e into main Jan 3, 2023
@gnapse gnapse deleted the ernesto/modal-stories branch January 3, 2023 12:41
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.

3 participants