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(modals): set motion preset to none #1783

Merged
merged 4 commits into from
Jan 31, 2024
Merged

Conversation

dcshzj
Copy link
Contributor

@dcshzj dcshzj commented Jan 23, 2024

Important

Requires testing on multiple devices + GSIB

Problem

The opening and closing of modals in a GSIB is extremely slow in the default rendering mode.

Solution

Breaking Changes

  • Yes - this PR contains breaking changes
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Bug Fixes:

  • Set the motionPreset for all modals to be none, so the modal will appear immediately instead of having an animation in which the modal slowly appears.

Tests

  • Unit tests (using npm run tests)
  • e2e tests (comment on this PR with the text !run e2e)
  • Smoke tests
    • Go to any site.
    • Click on every possible action, verify that modals that are opened and closed appear and disappear immediately without any animation.

Deploy Notes

None

alexanderleegs
alexanderleegs previously approved these changes Jan 24, 2024
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

code wise lgtm, but i'm wondering if we should create a custom component (in a separate pr) which extends the design-system modal so that we don't have to insert this field in every time, since we want all modals transitionless to better support gsib?

@kishore03109
Copy link
Contributor

+1 on @alexanderleegs, this feels dev heavy to remember. Would much rather use IsomerModal + do a string search for Modal to make sure we arent using the chakra's/design system (forgot which one) to ensure we didnt miss out anything!

@dcshzj
Copy link
Contributor Author

dcshzj commented Jan 24, 2024

@alexanderleegs @kishore03109 I think that is a good point, will use that approach instead!

@dcshzj dcshzj force-pushed the fix/modal-motion-none branch from 9641e35 to 69d079d Compare January 31, 2024 05:47
@dcshzj dcshzj force-pushed the fix/modal-motion-none branch from 69d079d to 0474149 Compare January 31, 2024 05:48
@dcshzj dcshzj dismissed alexanderleegs’s stale review January 31, 2024 06:16

Approach has changed

@dcshzj dcshzj requested a review from a team January 31, 2024 06:16
@dcshzj
Copy link
Contributor Author

dcshzj commented Jan 31, 2024

@isomerpages/iso-engineers re-requesting review to use the approach suggested above (i.e. to have our own Modal component that sets the motionPreset to none).

Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

Code lgtm. Just wondering if we should remove the parameter totally? Don't really have a strong opinion here

@dcshzj
Copy link
Contributor Author

dcshzj commented Jan 31, 2024

@seaerchin I'm keeping for now to maintain maximum compatibility, in case there are some use cases which still needs a custom motionPreset.

@dcshzj dcshzj merged commit 16ba106 into develop Jan 31, 2024
8 checks passed
@mergify mergify bot deleted the fix/modal-motion-none branch January 31, 2024 08:28
This was referenced Feb 8, 2024
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.

4 participants