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 money request tabs animation #29231

Merged
merged 10 commits into from
Oct 18, 2023
Merged

fix money request tabs animation #29231

Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
362a17f
fix tabs animation
ahmedGaber93 Oct 5, 2023
f3d797c
fix money request tabs animation
ahmedGaber93 Oct 10, 2023
0c1f2e1
fix money request tabs animation
ahmedGaber93 Oct 10, 2023
f0427e1
Merge branch 'main' into issue-27563
ahmedGaber93 Oct 10, 2023
a2b3e8e
Merge branch 'main' into issue-27563
ahmedGaber93 Oct 11, 2023
b5ec1f2
fix money request tabs animation
ahmedGaber93 Oct 12, 2023
878a8d3
fix money request tabs animation
ahmedGaber93 Oct 13, 2023
4863dd5
fix money request tabs animation
ahmedGaber93 Oct 13, 2023
3d8b9a2
prettier
ahmedGaber93 Oct 13, 2023
5504141
add comments
ahmedGaber93 Oct 18, 2023
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
31 changes: 23 additions & 8 deletions src/components/TabSelector/TabSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {View} from 'react-native';
import React from 'react';
import React, {useReducer, useRef} from 'react';
import PropTypes from 'prop-types';
import _ from 'underscore';
import * as Expensicons from '../Icon/Expensicons';
Expand Down Expand Up @@ -53,7 +53,7 @@ const getIconAndTitle = (route, translate) => {
}
};

const getOpacity = (position, routesLength, tabIndex, active) => {
const getOpacity = (position, routesLength, tabIndex, active, affectedTabs) => {
const activeValue = active ? 1 : 0;
const inactiveValue = active ? 0 : 1;

Expand All @@ -62,32 +62,46 @@ const getOpacity = (position, routesLength, tabIndex, active) => {

return position.interpolate({
inputRange,
outputRange: _.map(inputRange, (i) => (i === tabIndex ? activeValue : inactiveValue)),
outputRange: _.map(inputRange, (i) => (affectedTabs.includes(tabIndex) && i === tabIndex ? activeValue : inactiveValue)),
});
}
return activeValue;
};

const getBackgroundColor = (position, routesLength, tabIndex) => {
const getBackgroundColor = (position, routesLength, tabIndex, affectedTabs) => {
if (routesLength > 1) {
const inputRange = Array.from({length: routesLength}, (v, i) => i);

return position.interpolate({
inputRange,
outputRange: _.map(inputRange, (i) => (i === tabIndex ? themeColors.border : themeColors.appBG)),
outputRange: _.map(inputRange, (i) => (affectedTabs.includes(tabIndex) && i === tabIndex ? themeColors.border : themeColors.appBG)),
});
}
return themeColors.border;
};

function TabSelector({state, navigation, onTabPress, position}) {
const {translate} = useLocalize();
const [, reRender] = useReducer((x) => !x, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks kinda hack-ish. Wouldn't it be better to just keep affectedAnimatedTabs in state like below?

const defaultAffectedAnimatedTabs = Array.from({length: state.routes.length}, (v, i) => i);
const [affectedAnimatedTabs, setAffectedAnimatedTabs] = React.useState(defaultAffectedAnimatedTabs);

React.useEffect(() => {
    setTimeout(() => {
        setAffectedAnimatedTabs(defaultAffectedAnimatedTabs);
    }, CONST.ANIMATED_TRANSITION);
    // eslint-disable-next-line react-hooks/exhaustive-deps
}, [state.index]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@burczu
Yes, I think this is better.
Updated!


const defaultAffectedAnimatedTabs = Array.from({length: state.routes.length}, (v, i) => i);
const affectedAnimatedTabs = useRef(defaultAffectedAnimatedTabs);

React.useEffect(() => {
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We are usually against using setTimeout, why do we need it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need here to reset affectedAnimatedTabs after transition end, and I think this is the available way to wait transition end.

Removing setTimeout will back the issue to happened again because while moving from tab1 to tabs3 the transition is not end yet and at this time calling reset affectedAnimatedTabs to default will reset it to [tab1, tab2, tab3] while we need it to keep it [tab1, tab3] until transition end

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! Can you leave a comment then explaining why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pecanoro updated.

affectedAnimatedTabs.current = defaultAffectedAnimatedTabs;
// re-render is important to re-defining opacity and background base on new affected tabs, to be ready for user swiping.
reRender();
}, CONST.ANIMATED_TRANSITION);
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why we add this as we do here?

App/src/components/Form.js

Line 202 in 0cbee22

// eslint-disable-next-line react-hooks/exhaustive-deps -- we just want to revalidate the form on update if the preferred locale changed on another device so that errors get translated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add eslint-disable because putting defaultAffectedAnimatedTabs as deps will reset the affectedAnimatedTabs in every render, because const defaultAffectedAnimatedTabs will re-define the const in every render and is considered deps change because reference identity is changed.
I think we can remove eslint-disable and use useMemo

const defaultAffectedAnimatedTabs = useMemo(() => Array.from({length: state.routes.length}, (v, i) => i), [state.routes.length]);
    
 React.useEffect(() => {
    setTimeout(() => {
        setAffectedAnimatedTabs(defaultAffectedAnimatedTabs);
    }, CONST.ANIMATED_TRANSITION);
}, [defaultAffectedAnimatedTabs, state.index]);

}, [state.index]);

return (
<View style={styles.tabSelector}>
{_.map(state.routes, (route, index) => {
const activeOpacity = getOpacity(position, state.routes.length, index, true);
const inactiveOpacity = getOpacity(position, state.routes.length, index, false);
const backgroundColor = getBackgroundColor(position, state.routes.length, index);
const activeOpacity = getOpacity(position, state.routes.length, index, true, affectedAnimatedTabs.current);
const inactiveOpacity = getOpacity(position, state.routes.length, index, false, affectedAnimatedTabs.current);
const backgroundColor = getBackgroundColor(position, state.routes.length, index, affectedAnimatedTabs.current);
const isFocused = index === state.index;
const {icon, title} = getIconAndTitle(route.name, translate);

Expand All @@ -96,6 +110,7 @@ function TabSelector({state, navigation, onTabPress, position}) {
return;
}

affectedAnimatedTabs.current = [state.index, index];
const event = navigation.emit({
type: 'tabPress',
target: route.key,
Expand Down