-
Notifications
You must be signed in to change notification settings - Fork 347
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
Convert tournament from useRef to useState #2577
Convert tournament from useRef to useState #2577
Conversation
In the Tournament component, convert from a ref: const tournament_ref = useRef<TournamentInterface>(...); to a state: const tournament = useState<TournamentInterface>(...); This enabled the following additional changes: - Remove `refresh()`. - Removing redundant state, switching to `useMemo` for `sorted_players` and `raw_rounds` and local variables for others. - Reset non-tournament state when `tournament_id` changes. Depends on online-go#2575 (builds on top of it).
Uffizzi Preview |
Here's a link to incremental diff with just the changes from this PR: dexonsmith/online-go.com@fix-elimination-graph-to-update-on-changes...dexonsmith:online-go.com:convert-tournament-from-useref-to-usestate. I think the riskiest part of this is that all the editing things had to change to call As partial motivation, here are screenshots of how it used to work.
... But now, you just see the tournament. |
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 really like the changes in this PR - reactive state is a lot easier to process. Moving some functions out of the giant Tournament component looks good too.
I tested on this tournament - it's the one that would reveal any performance issues, and AFAICT everything still works as intended.
src/views/Tournament/Tournament.tsx
Outdated
refresh(); | ||
return t; | ||
}); | ||
void get(`tournaments/${tournament_id}`) |
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.
What's the purpose of void
before the promises?
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.
Relic. I think I was trying to satisfy the type checker (or something) I didn't understand why it was complaining. The problem was that I had not yet added .catch(...)
.
Cleaned up in af4783d.
src/views/Tournament/Tournament.tsx
Outdated
const [sorted_players, setSortedPlayers] = React.useState<any[]>([]); | ||
const [players, setPlayers] = React.useState<{ [id: string]: TournamentPlayer }>({}); | ||
const [is_joined, setIsJoined] = React.useState(false); | ||
const [clicked_round_idx, setClickedRoundIdx] = React.useState< |
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'm having trouble understanding the name change here. The term "Selected" is input-independent, whereas "clicked" refers to an input (replaced by touches or taps on mobile).
Further down, I see:
const setSelectedRound = (idx: number | "standings" | "roster") => {
setClickedRoundIdx(idx);
};
Which makes me think this should still be called "selected". And then maybe use "validated"/"sanitized" to distinguish the one down here.
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.
Okay. This one's a bit weird, I admit, and there's a behaviour change (which I think is mostly good, but maybe it's debatable).
Previously there was selected_round_idx
as state and a forwarding function that was used for setting it:
const [selected_round_idx, setSelectedRoundIdx] = React.useState(...);
const setSelectedRound = (idx: number) => setSelectedRoundIdx(idx);
(I left behind this forwarding function, it just points to setClickedRoundIdx
now. Maybe shouldn't have?)
The reason I've added an extra layer, "clicked", is to distinguish between:
- "is this selected because the user clicked on it?"
- "is this selected because we don't have any rounds loaded yet?"
Say a user opens a tournament that's currently in round 2 of 5. Here is the behaviour we we'll get right now:
- User opens the tournament. No rounds are displayed (but
selected_round_idx
will be 0). - Once the rounds load, round 2 (active round) is selected automatically.
- Round 2 finishes and round 3 starts. User automatically switched to round 3 (not having made a manual selection by clicking).
- User clicks on round 1, which selects round 1.
- Round 3 finishes and round 4 starts. User still has round 1 selected (having clicked on it).
- User clicks on round 4, which selects round 4.
- Round 4 finishes and round 5 starts. User still has round 4 selected (having clicked on it).
- User switches to a different tournament that's in round 1 via OGS search navigation. User gets reverted to (1).
Comments:
- (2) is important. It used to be accomplished by calling
setSelectedRoundIdx
any time rounds were loaded. Now the "default" round selection is just part of the assignment ofselected_round_idx
, and loading rounds doesn't change the state of what the user clicked on. - (3) is a behaviour I'm not sure if we want, but it's not a change. Comes naturally from (2). ("User can follow along a tournament as it's going")
- (5) is new behaviour. I think we want it. Accomplished by having
clicked_round_idx
start out asnull
(did they pick this round? or are they just following along?). - (7) is new behaviour. Maybe we don't want it. Maybe, instead, if they manually click on the active round, we should revert back to (3) by something like:
const setSelectedRound = (idx: number) => { setClickedRoundIdx(idx == default_round_idx ? null : idx); };
WDYT? Given that walkthrough, any other thoughts on naming? Should we change (7)? (Would this be clearer/easier to follow if we introduced a default_round_idx
computed value?)
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.
Made some changes: af4783d...856dd68. I think the code is more clear now, and it takes the change I suggested in my comment about (7) above.
Let me know what you think!
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.
Oh cool, thanks for the detailed explanation!
I think the behavior you specified sounds reasonable, and "explicitly selected" is a good name for that variable according to this description 🙂
…on-changes' into HEAD
…on-changes' into HEAD
Remove `void`s from promises. These were a relic from before I'd added the `.catch(...)` and I didn't understand why the type-checker was mad.
When a user selects an earlier round, it seems rude to force them forward if/when a new round comes in. But if they later click on the currently active round, revert them back to following along, so they don't get "stuck".
The same incremental diff should still work, since I merged from the branch for #2575. Here's the link again in case it's helpful: fix-elimination-graph-to-update-on-changes...convert-tournament-from-useref-to-usestate. |
Awesome, thanks! |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
@anoek, not sure what to do with this one; I don't have access to the target of the "View Issue" links. |
Fix [sentry issues](online-go#2577 (comment)) by delaying computation of rounds until everything is loaded.
In the Tournament component, convert from a ref:
to a state:
This enabled the following additional changes:
refresh()
.useMemo
forsorted_players
andraw_rounds
and local variables for others.tournament_id
changes.Depends on #2575 (builds on top of it). The diff here will be hard to read until that lands.