fix Modal animations by parasharrajat · Pull Request #2505 · Expensify/App

parasharrajat · 2021-04-21T00:04:55Z

Please review.

Details

Details can be read #2335 (comment)

Fixed Issues

Fixes #2335

Tests / QA Steps

Click the search icon to open the Search Modal.
Open all the available models from the FAB button.
Open the User Details / Group participants modal.
Open the User Settings Modal.
For all the modals enter and exit animation should be smooth and consistent.
Open Emoji Picker on Mobile devices and Web they should be animated.

Tested On

Screenshots

Web

animation-web.mp4

Mobile Web

animation-mWeb.mp4

Desktop

animation-des.mp4

iOS

Not able to record it as I have MAC VM but the animation on IOS works smoothly.

Android

animation-mobile.mp4

jasperhuangg · 2021-04-21T06:38:45Z

@parasharrajat As per the PR title, is this still WIP? Also, can you confirm whether you need to provide tests for Mobile Web, Desktop, and iOS?

parasharrajat · 2021-04-21T14:31:39Z

@jasperhuangg Yup main task is finished but I was scrutinizing it. Also, I saw other requests being brought into this issue.

…harrajat-animation

parasharrajat · 2021-04-26T03:58:07Z

@jasperhuangg Updated Screenshots. Ready for review.

jasperhuangg

A few questions.

jasperhuangg · 2021-04-26T05:07:33Z

                    numColumns={this.numColumns}
+                    removeClippedSubviews
+                    maxToRenderPerBatch={this.numColumns}
+                    windowSize={3}


What does this have to do with your issue? This just seems like a FlatList optimization that doesn't have anything do with your changes. NAB but I think it's better to do this in a separate PR.

Also, can you clarify why you're using 3 for windowSize? Not good to have hard coded numbers so good to clarify with a comment.

This component is very laggy. Author of the component mentioned this so he disabled the animations. I put these changes to make the animation smooth so that I can enable animation on this component.

Window size will not be reused so I put it directly over here. It just means to create three Windows where one window is equal to visible area of component. I will add the comments

I forgot to mention that there are other PRs on emoji picker which is optimizing it So I don't need to submit a PR for now. If in future issues persist with Emoji Picker I will handle them as a separate issue.

+1 for @jasperhuangg comments here.

@parasharrajat appreciate you adding value here but please do limit changes to ones affecting the issue you are assigned so that we can manage which bugs and issues are being handled and where. random fixes make it much harder to track when a bug was fixed.

I added these changes as per request #2335 (comment). I saw that the animation was pretty laggy so I thought to put these for optimizing the animation.

Should I just enable the animation and remove these changes?

Ah ok, thanks I missed this comment. No need to remove changes but if we are increasing the scope we need to add that context to the description of the original issue so we should:

Update the issue you are working on to show each problem we are solving (show that it is broken)

Ask QA to test the same flows and prove that it is fixed.

Otherwise, we're just adding random stuff and it's hard to have a good history.

Sure, Thanks. Updating now.

jasperhuangg · 2021-04-26T05:07:42Z

                    numColumns={this.numColumns}
+                    removeClippedSubviews
+                    maxToRenderPerBatch={this.numColumns}
+                    windowSize={3}


same as above

I was able to confirm locally that removing these line fixes a regression

jasperhuangg · 2021-04-26T05:10:56Z

                        onClose={this.hideEmojiPicker}
                        onModalShow={this.focusEmojiSearchInput}
                        hideModalContentWhileAnimating
-                        animationInTiming={1}


Why are we removing this? There's a comment above saying why they're there.

So It was added to disable the animation as the author found it laggy but I was requested to enable animation on emoji picker thus I removed it. I will remove the comments as well.

parasharrajat · 2021-04-29T02:16:25Z

@jasperhuangg Please let me know how should I proceed here based on my comments over review?

jasperhuangg

Looks good! Thanks for clarifying my concerns.

marcaaron · 2021-04-30T02:13:42Z

    showTitleTooltip: PropTypes.bool,
+
+    // The ref to the search input
+    // eslint-disable-next-line react/forbid-prop-types


Surely we will use this enough times that a reusable propTypes is better than PropTypes.object.isRequired ?

marcaaron · 2021-04-30T02:16:06Z


    componentDidMount() {
-        this.textInput.focus();
+        this.unsubscribeTransitionEnd = this.props.navigation.addListener('transitionEnd', () => {


What transition are we listening for the end of here?
Does this work for all screens that might use this component in the future?
Can we add some context about why this change was needed so that others have a better idea whether to follow this example or not?

Sure. We used this as Interaction Manager has issues on Web. This transition waits for the Navigation transition to finish. This component is being used for only Screen components so this works fine. But it's a good idea to mention it in a comment. Oh, PR is merged. I will open follow-up PR.

Let me know If we should handle this in a different way.

marcaaron · 2021-04-30T02:20:09Z

                    numColumns={this.numColumns}
+                    removeClippedSubviews
+                    maxToRenderPerBatch={this.numColumns}
+                    windowSize={3}


+1 for @jasperhuangg comments here.

@parasharrajat appreciate you adding value here but please do limit changes to ones affecting the issue you are assigned so that we can manage which bugs and issues are being handled and where. random fixes make it much harder to track when a bug was fixed.

marcaaron · 2021-04-30T02:22:16Z

-            this.emojiSearchInput.focus();
-        }
+        this.emojiFocusInteractionHandle = InteractionManager.runAfterInteractions(() => {
+            if (this.emojiSearchInput && !this.props.isSmallScreenWidth) {


Why only when we have a small screen width what about tablet ?

Yeah, this is a bigger device (!isSmallScreenWidth) but we can remove this check as emojiSearchInput is already null for native devices.

marcaaron · 2021-04-30T02:23:10Z

@@ -161,7 +170,7 @@ class ReportActionCompose extends React.Component {
        if (this.textInput) {
            // There could be other animations running while we trigger manual focus.
            // This prevents focus from making those animations janky.


Please elaborate on why this is helpful. Which "interaction" are we waiting to finish ?

So When the modal screen is being closed, we want to wait for those before focusing on it. And I noticed that manual focus is causing a lot of issues so I think it's better to put focus after Interactions, we have no side effects.

Is there some way to see which "interactions" we are waiting for? Or am I misunderstanding what this method does? Which janky animation are we fixing specifically with this? Is there a before/after video we can share to verify the change does what it should and so QA can verify the change as well?

I'm not seeing anything at all about the emoji picker in the test steps so those will also need to be updated.

+1 @parasharrajat Can you updated the test steps to include something about the emoji picker? It's not super apparent that it's a modal and it might get missed in QA.

Sure @jasperhuangg

OSBotify · 2021-04-30T02:38:53Z

🚀 Deployed to staging in version: 1.0.34-3🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2021-05-01T00:39:19Z

I believe this PR caused a regression

parasharrajat · 2021-05-01T00:42:54Z

I don't think so but I will debug it and see. anyways I am putting up a follow up PR for few changes so I will fix this regression (if its from same PR) on that

roryabraham · 2021-05-01T00:46:04Z

Yeah, I was just looking at the checklist here and finding that this PR looked like the only potential culprit, but I'm not able to reproduce the bug.

roryabraham · 2021-05-01T01:06:43Z

Okay, actually I have figured out how to reproduce the regression, and I'm 90% sure it was indeed caused by this PR.

roryabraham · 2021-05-01T01:15:26Z

Was able to confirm that these lines were the source of the regression. Going to revert here

parasharrajat · 2021-05-01T01:46:15Z

I am feeling like I need to test the app deeply before finalizing a PR. I am seeing that there have been couple of regressions.

roryabraham · 2021-05-03T16:00:10Z

@parasharrajat This PR has been reverted, so'll need to include these original changes in your follow-up too

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

platform	result
🤖 android 🤖	failure ❌
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

fix Modal animations

fca6966

parasharrajat requested a review from a team as a code owner April 21, 2021 00:04

MelvinBot requested review from jasperhuangg and removed request for a team April 21, 2021 00:05

jasperhuangg previously approved these changes Apr 21, 2021

View reviewed changes

Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…

073a6c7

…harrajat-animation

parasharrajat mentioned this pull request Apr 22, 2021

Switching between chats using LHN causes chats to get stuck offscreen #2051

Closed

parasharrajat added 4 commits April 23, 2021 12:54

Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…

60a769f

…harrajat-animation

chg: refactor Interaction Manager to Custom events

e5c453a

Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…

8e514ef

…harrajat-animation

chg: enabled animatons for Emoji Picker

419c9d6

parasharrajat dismissed jasperhuangg’s stale review via 419c9d6 April 26, 2021 02:19

parasharrajat changed the title ~~WIP: fix Modal animations~~ fix Modal animations Apr 26, 2021

jasperhuangg requested changes Apr 26, 2021

View reviewed changes

jasperhuangg approved these changes Apr 30, 2021

View reviewed changes

jasperhuangg merged commit c31821b into Expensify:main Apr 30, 2021

OSBotify mentioned this pull request Apr 30, 2021

Deploy Checklist: Expensify.cash 2021-04-29 #2639

Closed

23 tasks

marcaaron reviewed Apr 30, 2021

View reviewed changes

marcaaron assigned parasharrajat Apr 30, 2021

roryabraham mentioned this pull request May 1, 2021

Revert "fix Modal animations" #2663

Merged

parasharrajat mentioned this pull request May 3, 2021

Modal animation is inconsistent #2335

Closed

Conversation

parasharrajat commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Tests / QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

jasperhuangg commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parasharrajat commented Apr 21, 2021

Uh oh!

parasharrajat commented Apr 26, 2021

Uh oh!

jasperhuangg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Apr 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Apr 29, 2021

Uh oh!

jasperhuangg left a comment

Choose a reason for hiding this comment

Uh oh!

marcaaron Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat commented Apr 21, 2021 •

edited

Loading

jasperhuangg commented Apr 21, 2021 •

edited

Loading

jasperhuangg left a comment •

edited

Loading

parasharrajat Apr 26, 2021 •

edited

Loading

parasharrajat Apr 26, 2021 •

edited

Loading

parasharrajat Apr 30, 2021 •

edited

Loading

marcaaron Apr 30, 2021 •

edited

Loading

parasharrajat commented May 1, 2021 •

edited

Loading

roryabraham commented May 1, 2021 •

edited

Loading

roryabraham commented May 1, 2021 •

edited

Loading