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

Jump to bottom

MM-59973: fix the emoji count 0 issue when archive and unarchive chan… #8146

Merged
merged 12 commits into from
Oct 23, 2024
Merged

MM-59973: fix the emoji count 0 issue when archive and unarchive chan… #8146

Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
4b12bc7
MM-59973: fix the emoji count 0 issue when archive and unarchive chan…
Rajat-Dabade Aug 13, 2024
9a0b74a
ref to null and initialing it
Rajat-Dabade Sep 30, 2024
6e08ff6
refactor: added a safe condition
Rajat-Dabade Sep 30, 2024
6fea662
refactor code
Rajat-Dabade Oct 1, 2024
aac779d
more clean up
Rajat-Dabade Oct 1, 2024
e4843fb
clean-ups
Rajat-Dabade Oct 1, 2024
5ed7630
index to tosting as a key to component
Rajat-Dabade Oct 1, 2024
955c04f
refactor code
Rajat-Dabade Oct 2, 2024
e2a4233
fix: (MM-59973) emoji count 0 update (#8256)
rahimrahman Oct 15, 2024
d144114
Merge branch 'main' into MM-59973-emoji-count-0
mattermost-build Oct 21, 2024
3862eeb
Test: increase timeout-minutes to 120 for e2e test
Rajat-Dabade Oct 22, 2024
00d9cb4
reverted the timeout-minutes back to 40
Rajat-Dabade Oct 23, 2024
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
100 changes: 48 additions & 52 deletions app/components/animated_number/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ interface Props {
animateToNumber: number;
fontStyle?: StyleProp<TextStyle>;
animationDuration?: number;
easing?: ((input: number) => number) | undefined;
easing?: number;
}

const NUMBERS = Array(10).fill(null).map((_, i) => i);
Expand All @@ -26,43 +26,43 @@ const AnimatedNumber = ({
animateToNumber,
animationDuration,
fontStyle,
easing,
easing = 1.2,
}: Props) => {
const prevNumber = usePrevious(animateToNumber);
const animateToNumberString = String(Math.abs(animateToNumber));
const prevNumberString = String(Math.abs(prevNumber));

const numberStringToDigitsArray = Array.from(animateToNumberString, Number);
const prevNumberersArr = Array.from(prevNumberString, Number);

const [numberHeight, setNumberHeight] = React.useState(0);
const animations = useMemo(() => numberStringToDigitsArray.map((__, index) => {
if (typeof prevNumberersArr[index] !== 'number') {
return new Animated.Value(0);
const animations = useMemo(() => {
if (numberHeight === 0) {
return [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: would it be safer to return

    return numberStringToDigitsArray.fill(new Animated.Value(0));

just to protect from future regression?

Could transform.translateY: undefined

                              translateY: animations[index],

cause any issue if index is out of bounds due to animations being empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I think numberStringToDigitsArray is a number array and cannot be filled with Value type. Maybe map can work here to return the new array filled with Animated.Value(0).

}
const numberStringToDigitsArray = Array.from(animateToNumberString, Number);
const prevNumberersArr = Array.from(prevNumberString, Number);

const animationHeight = -1 * (numberHeight * prevNumberersArr[index]);
return new Animated.Value(animationHeight);
}), [numberStringToDigitsArray]);
return numberStringToDigitsArray.map((digit, index) => {
// Skip animation if the current and previous digits are the same
if (prevNumberersArr[index] === digit) {
return new Animated.Value(-1 * (numberHeight * digit));
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Could this cause issues when prevNumber = 99 and animateToNumber = 100? In this case, numberStringToDigitsArray = [1, 0, 0], but prevNumberersArr = [9, 9]. On the first digit, prevNumberersArr[0] !== 1, so the condition will fail. Could this behavior cause issues during a rapid animation test (e.g., moving +1 every 100ms)?

  2. (NIT) This comment seems redundant since the logic is already clear from the code itself

             if (prevNumberersArr[index] === digit) {

Following the DRY principle, you could remove it. However, I think a comment explaining the next line

              // skipping any animation by just returning the current "position"
              return new Animated.Value(-1 * (numberHeight * digit));

...could add value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this condition could indeed cause an issue when transitioning from 99 to 100. The problem arises because we skip the animation when the digits are the same. For example:

  • prevNumberersArr = [9, 9]
  • numberStringToDigitsArray = [1, 0, 0]

When comparing the first digit (1 vs 9), the condition prevNumberersArr[0] === digit will fail, which is expected, and the animation will proceed for the first digit. However, for the second and third digits, the condition prevNumberersArr[1] === digit will compare 9 (previous) to 0 (current), which is not equal, so those digits will still be animated, which is correct.

The issue is not necessarily in this block itself, but the logic could behave unexpectedly if we are rapidly changing values (e.g., every 100ms). Skipping digits could lead to an incorrect visual transition, especially when the change is rapid, such as going from 99 to 100.

To ensure the conditions for rapid changes are avoided we can add an extra condition for checking whether the length of the prevNumberersArr and numberStringToDigitsArray are equal or not which will ensure that the animation isn't skipped due to rapid changes.

}

const setButtonLayout = useCallback((e: LayoutChangeEvent) => {
setNumberHeight(e.nativeEvent.layout.height);
}, []);
const prevHeight = -1 * (numberHeight * (prevNumberersArr[index] || 0));
const animation = new Animated.Value(prevHeight);

React.useEffect(() => {
animations.forEach((animation, index) => {
Animated.timing(animation, {
toValue: -1 * (numberHeight * numberStringToDigitsArray[index]),
duration: animationDuration || 1400,
toValue: -1 * (numberHeight * digit),
duration: animationDuration,
rahimrahman marked this conversation as resolved.
Show resolved Hide resolved
useNativeDriver: true,
easing: easing || Easing.elastic(1.2),
easing: Easing.elastic(easing),
}).start();

return animation;
});
}, [animateToNumber, animationDuration, fontStyle, numberHeight]);
}, [animateToNumberString, prevNumber, numberHeight, animationDuration, easing]);
Copy link
Contributor

Choose a reason for hiding this comment

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

prevNumber is not a dependency of this useMemo but prevNumberString is. I know they are both related, but for the sake of not triggering "exhaustive-deps" warning (it is currently disabled), I recommend using the correct dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the dependency.


const getTranslateY = (index: number) => {
return animations[index];
};
const setButtonLayout = useCallback((e: LayoutChangeEvent) => {
setNumberHeight(e.nativeEvent.layout.height);
}, []);

return (
<>
Expand All @@ -71,37 +71,33 @@ const AnimatedNumber = ({
{animateToNumber < 0 && (
<Text style={[fontStyle, {height: numberHeight}]}>{'-'}</Text>
)}
{numberStringToDigitsArray.map((n, index) => {
return (
<View
key={`${index.toString()}`}
style={{height: numberHeight, overflow: 'hidden'}}
>
<Animated.View
style={[
{Array.from(animateToNumberString, Number).map((_, index) => (
<View
key={`${index.toString()}`}
style={{height: numberHeight, overflow: 'hidden'}}
>
<Animated.View
style={{
transform: [
{
transform: [
{
translateY: getTranslateY(index),
},
],
translateY: animations[index],
},
]}
>
{NUMBERS.map((number, i) => (
<View
style={{flexDirection: 'row'}}
key={`${i.toString()}`}
>
<Text style={[fontStyle, {height: numberHeight}]}>
{number}
</Text>
</View>
))}
</Animated.View>
</View>
);
})}
],
}}
>
{NUMBERS.map((number, i) => (
<View
key={`${i.toString()}`}
style={{flexDirection: 'row'}}
>
<Text style={[fontStyle, {height: numberHeight}]}>
{number}
</Text>
</View>
))}
</Animated.View>
</View>
))}
</View>
)}
{numberHeight === 0 &&
Expand Down