[UX Reliability] Improve the bottom modal animations#55157
Conversation
…-native-modal-refactor
…oszGrajdek/react-native-modal-refactor-Vkuba # Conflicts: # src/pages/home/report/ContextMenu/ContextMenuActions.tsx
…act-native-modal-refactor-Vkuba adding some types, and do some refactor
fix trashold
…-native-modal-refactor
…oszGrajdek/react-native-modal-refactor_kuba_v2 # Conflicts: # src/components/Search/SearchRouter/SearchRouterModal.tsx
…oszGrajdek/react-native-modal-refactor_kuba_v2 # Conflicts: # src/components/Modal/BaseModal.tsx
@bartosz grajdek/react native modal refactor kuba v2
BartoszGrajdek
changed the title
|
Looks nice and smooth on my device @BartoszGrajdek can you please resolve merge conflicts? |
|
At first everything looks nice for me too The only thing I noticed on the desktop is this 2025-02-17.16.13.57.mov |
|
@ZhenjaHorbach This bug should be solved in #56594 |
This modal is not affected by these changes, the only modal that will use new code for now is FAB, we'll gradually implement the new version in other modals later. |
Strange |
Oh |
|
@BartoszGrajdek Conflicts have to be resolved before merging |
|
Fixed @mountiny let's see if all the checks pass and it should be ready to go 😄 |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.1.1-0 🚀
|
|
Team is facing #57081 when running this PR |
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.1.1-6 🚀
|
| width: deviceWidthProp ?? deviceWidth, | ||
| height: deviceHeightProp ?? deviceHeight, | ||
| backgroundColor: backdropColor, | ||
| opacity: getPlatform() === CONST.PLATFORM.WEB ? backdropOpacity : 1, |
There was a problem hiding this comment.
This platform specific condition led to the following issue on Desktop app:
- [Due for payment 2025-05-20] [$250] Desktop - "Select" modal is shown with a blank background #61003
which we fixed in this PR by removing the platform specific condition and rewriting the logic to use existing Backdrop index.web.tsx platform specific file.
| width: deviceWidthProp ?? deviceWidth, | ||
| height: deviceHeightProp ?? deviceHeight, |
There was a problem hiding this comment.
Using state here and calculating device width / height using Dimensions window led to the follosing issue:
which we fixed be removing the set state logic and using Dimensions screen instead:
const {width, height} = Dimensions.get('screen');
There was a problem hiding this comment.
Adding to the above ^ We should probably have used the useWindowDimensions hook because the Dimensions.get('screen') gives us the screen size and not necessary the viewport size, e.g. if you scale the window, you'd get a different viewport size but the screen size won't change.
(Coming from #68298)
|
|
||
| useEffect(() => { | ||
| const deviceEventListener = DeviceEventEmitter.addListener('didUpdateDimensions', handleDimensionsUpdate); | ||
| if (getPlatform() === CONST.PLATFORM.WEB) { |
There was a problem hiding this comment.
We should have checked this for desktop as well. Not doing so caused #61206
| accessible | ||
| accessibilityLabel={translate('modal.backdropLabel')} | ||
| onPress={onBackdropPress} | ||
| > |
| setIsTransitioning(true); | ||
| } else if (!isVisible && isContainerOpen && !isTransitioning) { | ||
| onModalWillHide(); | ||
|
|
There was a problem hiding this comment.
Coming from #68191. We should blur active element when hiding the model otherwise the input will be refocused
16 participants
Explanation of Change
This PR reimplements the library
react-native-modalthat we've been using for all modals but withreact-native-reanimatedinstead ofreact-native-animatableinside of it.We'll gradually switch to the new solution, going through each type of bottom-docked modals. In this PR, we're adding the refactored and changed code and implementing the new solution in FAB.
Fixed Issues
$ #49354
Tests
This PR affects just the FAB modal on native & mWeb platforms.
I think testing should go like this:
Offline tests
N/A - nothing should change in this case
QA Steps
Same as in the Tests section
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android.mov
Android: mWeb Chrome
Android.mWeb.mov
iOS: Native
iOS.mov
iOS: mWeb Safari
iOS.mWeb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov