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.

Sign up for GitHub

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 Modal animations #2505

Merged
merged 6 commits into from
Apr 30, 2021
Merged

fix Modal animations #2505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fca6966
fix Modal animations
parasharrajat Apr 15, 2021
073a6c7
Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…
parasharrajat Apr 21, 2021
60a769f
Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…
parasharrajat Apr 23, 2021
e5c453a
chg: refactor Interaction Manager to Custom events
parasharrajat Apr 23, 2021
8e514ef
Merge branch 'main' of github.com:Expensify/Expensify.cash into paras…
parasharrajat Apr 23, 2021
419c9d6
chg: enabled animatons for Emoji Picker
parasharrajat Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/components/OptionsSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import _ from 'underscore';
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import {withNavigation} from '@react-navigation/compat';
import TextInputWithFocusStyles from './TextInputWithFocusStyles';
import OptionsList from './OptionsList';
import styles from '../styles/styles';
Expand Down Expand Up @@ -59,6 +60,10 @@ const propTypes = {

// Whether to show the title tooltip
showTitleTooltip: PropTypes.bool,

// The ref to the search input
// eslint-disable-next-line react/forbid-prop-types
Copy link
Contributor

@marcaaron marcaaron Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

navigation: PropTypes.object.isRequired,
};

const defaultProps = {
Expand Down Expand Up @@ -87,7 +92,13 @@ class OptionsSelector extends Component {
}

componentDidMount() {
this.textInput.focus();
this.unsubscribeTransitionEnd = this.props.navigation.addListener('transitionEnd', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

this.textInput.focus();
});
}

componentWillUnmount() {
this.unsubscribeTransitionEnd();
}

/**
Expand Down Expand Up @@ -204,4 +215,4 @@ class OptionsSelector extends Component {

OptionsSelector.defaultProps = defaultProps;
OptionsSelector.propTypes = propTypes;
export default OptionsSelector;
export default withNavigation(OptionsSelector);
2 changes: 1 addition & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ class AuthScreens extends React.Component {
const modalScreenOptions = {
headerShown: false,
cardStyle: getNavigationModalCardStyle(this.props.isSmallScreenWidth),
cardStyleInterpolator: modalCardStyleInterpolator,
cardStyleInterpolator: (...props) => modalCardStyleInterpolator(this.props.isSmallScreenWidth, ...props),
animationEnabled: true,
gestureDirection: 'horizontal',
cardOverlayEnabled: true,
Expand Down
18 changes: 11 additions & 7 deletions src/libs/Navigation/AppNavigator/modalCardStyleInterpolator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import {Animated} from 'react-native';
import variables from '../../../styles/variables';

export default ({
current: {progress},
inverted,
layouts: {
screen,
export default (
isSmallScreen,
{
current: {progress},
inverted,
layouts: {
screen,
},
},
}) => {
) => {
const translateX = Animated.multiply(progress.interpolate({
inputRange: [0, 1],
outputRange: [screen.width, 0],
outputRange: [isSmallScreen ? screen.width : variables.sideBarWidth, 0],
extrapolate: 'clamp',
}), inverted);

Expand Down
3 changes: 3 additions & 0 deletions src/pages/home/report/EmojiPickerMenu/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ class EmojiPickerMenu extends Component {
renderItem={this.renderItem}
keyExtractor={item => `emoji_picker_${item.code}`}
numColumns={this.numColumns}
removeClippedSubviews
maxToRenderPerBatch={this.numColumns}
windowSize={3}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@parasharrajat parasharrajat Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@parasharrajat parasharrajat Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

Copy link
Member Author

@parasharrajat parasharrajat Apr 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Update the issue you are working on to show each problem we are solving (show that it is broken)
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, Thanks. Updating now.

style={styles.emojiPickerList}
extraData={this.state.filteredEmojis}
stickyHeaderIndices={this.state.headerIndices}
Expand Down
3 changes: 3 additions & 0 deletions src/pages/home/report/EmojiPickerMenu/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ class EmojiPickerMenu extends Component {
renderItem={this.renderItem}
keyExtractor={item => (`emoji_picker_${item.code}`)}
numColumns={this.numColumns}
removeClippedSubviews
maxToRenderPerBatch={this.numColumns}
windowSize={3}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor

Choose a reason for hiding this comment

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

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

style={styles.emojiPickerList}
stickyHeaderIndices={this.unfilteredHeaderIndices}
/>
Expand Down
21 changes: 15 additions & 6 deletions src/pages/home/report/ReportActionCompose.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ class ReportActionCompose extends React.Component {
}
}

componentWillUnmount() {
if (this.emojiFocusInteractionHandle) {
this.emojiFocusInteractionHandle.cancel();
}
if (this.textInputFocusInteractionHandle) {
this.textInputFocusInteractionHandle.cancel();
}
}

/**
* Updates the Highlight state of the composer
*
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure @jasperhuangg

InteractionManager.runAfterInteractions(() => {
this.textInputFocusInteractionHandle = InteractionManager.runAfterInteractions(() => {
this.textInput.focus();
});
}
Expand Down Expand Up @@ -249,9 +258,11 @@ class ReportActionCompose extends React.Component {
* Focus the search input in the emoji picker.
*/
focusEmojiSearchInput() {
if (this.emojiSearchInput) {
this.emojiSearchInput.focus();
}
this.emojiFocusInteractionHandle = InteractionManager.runAfterInteractions(() => {
if (this.emojiSearchInput && !this.props.isSmallScreenWidth) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

this.emojiSearchInput.focus();
}
});
}

/**
Expand Down Expand Up @@ -397,8 +408,6 @@ class ReportActionCompose extends React.Component {
onClose={this.hideEmojiPicker}
onModalShow={this.focusEmojiSearchInput}
hideModalContentWhileAnimating
animationInTiming={1}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

animationOutTiming={1}
anchorPosition={{
top: this.state.emojiPopoverAnchorPosition.vertical - CONST.EMOJI_PICKER_SIZE,
left: this.state.emojiPopoverAnchorPosition.horizontal - CONST.EMOJI_PICKER_SIZE,
Expand Down