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.

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: (MM-59973) emoji count 0 update #8256

Merged

Conversation

rahimrahman
Copy link
Contributor

  • tried to include everything into useMemo was an anti-pattern
  • moved back to useMemo + useEffect
  • previousNumber and animateToNumber comparison has some flaw when dealing with difference length between the two

Summary

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

This PR was tested on:

Screenshots

Release Note


* tried to include everything into useMemo was an anti-pattern
* moved back to useMemo + useEffect
* previousNumber and animateToNumber comparison has some flaw when dealing with difference length between the two
@mm-cloud-bot mm-cloud-bot added kind/bug Categorizes issue or PR as related to a bug. release-note labels Oct 11, 2024
@rahimrahman
Copy link
Contributor Author

rahimrahman commented Oct 11, 2024

@Rajat-Dabade I made this PR to not clutter yours and the base branch is yours. I reverted your changes that combined useEffect into useMemo. After talking to @larkox, I agreed that animation effects should stay in useEffect because of

  1. separation of concern
  2. readability

While I think if we had combined them, it would have been made so much simpler, and less code, and even slightly performant. However, the reasons above are definitely more ideal. Even ChatGPT agrees: https://chatgpt.com/share/66fed195-d0b0-8013-bec5-afb87b680030.

I've also made a bit more changes with the intent to make things "cleaner". I've noticed that when animateToNumber changed to 1000, previousNumber is 999, the original was comparing the wrong digit. You made some change, but I thought the previousNumberString memoized would make the number 0999 so it compares digit by digit (without having to do other type of checking (length comparison)).

999 => 1000, then 999 becomes 0999
1000 => 999, then 1000 becomes 000.
100033 => 21, then 100033 becomes just 33.

In the last example, if the animateToNumber is 21, we only run the loop twice, and index 0 and 1 will match perfectly to 3 and 3 => 2 and 1.

Let me know your thoughts on this and/or the rest of the changes.

@@ -76,7 +100,7 @@ const AnimatedNumber = ({
)}
{Array.from(animateToNumberString, Number).map((_, index) => (
<View
key={`${index.toString()}`}
key={`${animateToNumberString.length - 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.

Was using index as a key, but changing from 99 => 199, were causing the original two views to be re-rendered when it is not necessary.

By using the reverse index as key, hopefully the first 2 views will stay intact, and React only have to add 1 additional View tree.

Keep in mind, there are may Views (10) within each View, so not causing the parent View to re-render is a good thing.

Copy link
Contributor

@Rajat-Dabade Rajat-Dabade left a comment

Choose a reason for hiding this comment

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

@rahimrahman, Thanks for the update and for creating the PR! I totally understand the reasoning behind reverting the useMemo to useEffect based on the separation of concerns and readability. I agree that keeping the animation effect in useEffect aligns better with React's principles, especially when handling side effects.

Regarding the changes with the previous number comparison logic, I like the idea of using previousNumberString with padding to compare digit by digit without extra checks. It feels cleaner and handles the edge cases well.

Nice work 🚀 .

'0',
);

if (_previousNumberString.length > animateToNumberString.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rahimrahman, in which case this code will execute, as we are adding padding to the string with 0, this means the _previousNumberString and anumateToNumberString lengths will always be equal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rajat-Dabade When it goes from 1000 to 999 or something extreme like 100000001 to 1. The AnimateNumber doesn't really know what's being fed into it, so we have to be ready for any scenarios.

Take 1000 to 999 as an example. The padStart() only adds leading 0 to the length of animateToNumberString.length to _previousNumberString (not to animateToNumberString). In this case, it sliced 1000 to match the length of 999 thus becoming 000. Then this component knows to animate from 0 to 9 in all 3 digits.

I actually left a comment. If it wasn't clear, maybe we need to tweak it.

@rahimrahman
Copy link
Contributor Author

@Rajat-Dabade feel free to merge this into your PR at any time. This should not require 2 approval as it is going to be merged into your PR.

@Rajat-Dabade Rajat-Dabade merged commit e2a4233 into MM-59973-emoji-count-0 Oct 15, 2024
4 checks passed
@Rajat-Dabade Rajat-Dabade deleted the fix/MM-59973-emoji-count-0-update branch October 15, 2024 07:52
Rajat-Dabade added a commit that referenced this pull request Oct 23, 2024
#8146)

* MM-59973: fix the emoji count 0 issue when archive and unarchive channels

* ref to null and initialing it

* refactor: added a safe condition

* refactor code

* more clean up

* clean-ups

* index to tosting as a key to component

* refactor code

* fix: (MM-59973) emoji count 0 update (#8256)

* reversed from all useMemo to useMemo + useEffect
* tried to include everything into useMemo was an anti-pattern
* moved back to useMemo + useEffect
* previousNumber and animateToNumber comparison has some flaw when dealing with difference length between the two

* simple fix

* fix more silly mistakes

* move _previousNumberString to constant

* Test: increase timeout-minutes to 120 for e2e test

* reverted the timeout-minutes back to 40

---------

Co-authored-by: Rahim Rahman <rahim.rahman@mattermost.com>
Co-authored-by: Mattermost Build <build@mattermost.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants