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

fix: React 17 fix - replace react-transition-group in draft-modal #1301

Merged
merged 9 commits into from Apr 7, 2021

Conversation

lijuenc
Copy link

@lijuenc lijuenc commented Mar 22, 2021

Objective

Replace CSSTransition from react-transition-group with Transition from headlessui/react. The API is very close making it a near drop-in replacement.

For this PR, the relevant change happens in GenericModal.scss and GenericModal.tsx. There is also a minor change to Modal.elm to make the CSS class names consistent.

The rest of the PR is adding tests to GenericModal and the presets we have (ConfirmationModal, InformationModal, InputEditModal, RoadblockModal) to hopefully catch issues like this in the future.

Motivation and Context

react-transition-group is not actively maintained (reactjs/react-transition-group#630 (comment)) and is not fully compatible with React 17 (#1184).

Checklist

  • If this contains visual changes, has it been reviewed by a designer? N/A
  • If this introduces either a new component or a breaking change to an existing component, has it been reviewed by an advocate? (Ask in #prod_design_systems)
  • I have considered likely risks of these changes and got someone else to QA as appropriate
  • I have or will communicate these changes to the front end practice

Additional Considerations

  • Has the test suite been updated?
    A basic test case has been added for GenericModal, however, we would like to have the test suite running in both React 16 and React 17 to catch issues such as this.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 22, 2021

✨ Here are your branch previews! ✨

Last updated for commit 52d432d: Merge branch 'master' into modal-react-17

@lijuenc lijuenc force-pushed the modal-react-17 branch 2 times, most recently from 5ba6ce6 to 8131af0 Compare March 22, 2021 05:45
@lijuenc lijuenc changed the title Replace react-transition-group with headlessui/react transition fix: React 17 fix - replace react-transition-group with headlessui/react Mar 22, 2021
@lijuenc lijuenc changed the title fix: React 17 fix - replace react-transition-group with headlessui/react fix: React 17 fix - replace react-transition-group Mar 22, 2021
@lijuenc lijuenc force-pushed the modal-react-17 branch 2 times, most recently from 310f1a2 to 6025d1d Compare April 7, 2021 00:54
@lijuenc lijuenc marked this pull request as ready for review April 7, 2021 00:58
@lijuenc lijuenc changed the title fix: React 17 fix - replace react-transition-group fix: React 17 fix - replace react-transition-group in draft-modal Apr 7, 2021
Copy link
Contributor

@ActuallyACat ActuallyACat left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@lijuenc lijuenc merged commit 92899f7 into master Apr 7, 2021
@lijuenc lijuenc deleted the modal-react-17 branch April 7, 2021 06:22
dmisdm pushed a commit that referenced this pull request Apr 21, 2021
)

* fix: React 17 fix - replace react-transition-group

react-transition-group is not actively maintained and is not fully
compatible with React 17

* Added tests to Confirmation modal

* Reformat - fix linting errors

* Fix typo

* Add some tests for GenericModal

* Add some tests for InformationModal

* Add some tests for InputEditModal

* Add some tests for RoadblockModal

Co-authored-by: actuallyacat <allanna.beaton@gmail.com>
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.

None yet

2 participants