[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
BartoszGrajdek
changed the title
| }; | ||
|
|
||
| function ModalComponent({type, shouldUseNewModal, ...props}: ModalComponentProps) { | ||
| if (type === CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED && shouldUseNewModal) { |
There was a problem hiding this comment.
If we control all the modals with shouldUseNewModal then is type comparison needed too?
src/styles/index.ts
Outdated
| paddingTop: 0, // adjusting this because the mobile modal adds additional padding that we don't need for our layout | ||
| maxHeight: '95%', | ||
| }, | ||
| maxHeight: isSmallScreenWidth ? '100%' : '95%', |
There was a problem hiding this comment.
What is this magic? 🪄 95% seems random
There was a problem hiding this comment.
AFAIK This is a fix to a bug where in EmojiModal after focusing the search input the modal + keyboard took so much space that the user couldn't click the backdrop & exit modal. I still need to look into it though, maybe there's a better solution for this issue
…-native-modal-refactor
|
@BartoszGrajdek I think it would be nice to only migrate one instance of the modal for now so we dont need to have very large pr to revert in case there are some blockers. Lets just do one and ask QA to test it thoroughly in staging and then do the others if we are all good |
|
@mountiny sure - makes sense. In that case, I'll make the appropriate changes tomorrow 👀 |
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 components that use
PopoverWithMeasuredConent(excludingButtonWithDropdownMenusince it has quite a few use cases).Fixed Issues
$
PROPOSAL:
Tests
I think testing of each modal should go like this:
I'll now go through each modal we're going to change in this PR and explain how you can reach it.
EmojiPicker
You can also reach it through message reactions like so:
PopoverReportActionContextMenu
BasePopoverReactionList
ThreeDotsMenu
There's multiple instances of
ThreeDotsMenuaround the app i.e.:AvatarWithImagePicker
Prerequisite: you need an avatar image, the default Expensify profile pictures don't work
FloatingActionButtonAndPopover
AttachmentPickerWithMenuItems
AccountSwitcher
VideoPopoverMenu
SearchTypeMenuNarrow
SecuritySettingsPage
AddPaymentMethodMenu
I didn't manage to get it opened through normal methods, so hard-coded it to appear in this case.
ConnectToNetSuiteFlow
Prerequisite: You need a net suite integration set up first
ConnectToSageIntacctFlow
Prerequisite: You need a sage intacct integration set up first
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: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop