-
Notifications
You must be signed in to change notification settings - Fork 226
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
Reduce lag when toggling sets #1837
Conversation
[frontend] [Tue Mar 26 18:15:58 UTC 2024] - Deployed 929aa5a to https://genshin-optimizer-prs.github.io/pr/1837/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Tue Mar 26 18:21:47 UTC 2024] - Deployed 249ed9d to https://genshin-optimizer-prs.github.io/pr/1837/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Mar 27 02:55:17 UTC 2024] - Deployed 8c031ba to https://genshin-optimizer-prs.github.io/pr/1837/frontend (Takes 3-5 minutes after this completes to be available) [frontend] [Wed Apr 3 01:59:06 UTC 2024] - Deployed f115bc8 to https://genshin-optimizer-prs.github.io/pr/1837/frontend (Takes 3-5 minutes after this completes to be available) |
* React compares objects by pointer equality, so `[...new Set(xs)].filter(f)` | ||
* always causes a rerender | ||
*/ | ||
function uniqueFilterInplace<T>(xs: T[], f: (x: T) => boolean): T[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it may make sense to use React.memo
's arePropsEqual
to do a JSON string comparison on the UI update side, instead of "conserving references" in database (which kind of complicates the db logic).
apps/frontend/src/app/PageTeam/CharacterDisplay/Tabs/TabOptimize/index.tsx
Show resolved
Hide resolved
<SqBadge color="info"> | ||
<AtkIcon {...iconInlineProps} /> {mainStatSlotTots.plume.atk} | ||
</SqBadge> | ||
return useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way to go is to convert this into a React.memo
function, and propdrill in mainStatKeys
, and use arePropsEqual
(compare JSON string) to check for update.
Looks like this change has became too stale. I do support the changes you are making, but it seems that the underlying code has shifted too much. |
This code is likely too stale (4 months old). Will close for now. |
Describe your changes
Updating set effects is still delayed (e.g toggle Bloodstained Chivalry); this was previously hidden by the lag
Issue or discord link
Fix #1823
Testing/validation
Checklist before requesting a review (leave this PR as draft if any part of this list is not done.)
yarn run mini-ci
locally to validate format and lint.