mattermost / mattermost-mobile

Next generation iOS and Android apps for Mattermost in React Native
https://about.mattermost.com/
Apache License 2.0
2.26k stars 1.37k forks source link

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

Closed rahimrahman closed 1 month ago

rahimrahman commented 1 month ago

Summary

Ticket Link

Checklist

Device Information

This PR was tested on:

Screenshots

Release Note

rahimrahman commented 1 month ago

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

rahimrahman commented 1 month ago

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