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

Update how splits are loaded from the leaderboard #363

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Update how splits are loaded from the leaderboard #363

wants to merge 11 commits into from

Conversation

wooferzfg
Copy link
Member

  • The Run Editor now shows a loading spinner while it's loading splits from the leaderboard.
  • I added a new property to the Run Editor menu state, persistChanges. This determines whether changes made in the Run Editor should immediately be saved to the database or not. We were previously using the splits key to determine this, but we now need that in order to switch between runs while the Run Editor is open.

Fix #362

@wooferzfg wooferzfg requested a review from CryZe May 12, 2020 03:12
@wooferzfg
Copy link
Member Author

wooferzfg commented May 12, 2020 •
edited
Loading

@CryZe What do we want the behavior to be in this situation?

5dSeawif1t

Currently, I'm giving the imported splits an undefined splits key, which means they don't automatically get persisted to the database.

wooferzfg
wooferzfg commented May 12, 2020
View reviewed changes
src/ui/RunEditor.tsx Outdated
@@ -1858,6 +1872,18 @@ export class RunEditor extends React.Component<Props, State> {
}

private async downloadSplits<T>(apiRun: Run<T>, apiUri: string) {
// TODO: Determine whether there are any unsaved changes in the Run Editor
Copy link
Member Author

Choose a reason for hiding this comment

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

I think being able to close the Run Editor without disposing it would allow us to handle this?

CryZe
CryZe reviewed May 12, 2020
View reviewed changes
src/ui/TimerView.tsx Outdated
@@ -246,7 +246,7 @@ export class TimerView extends React.Component<Props, State> {
toast.error(e);
};
this.connection.onmessage = (e) => {
// FIXME: Clone the Shared Timer. This assumes that `this` is always
// TODO: Clone the Shared Timer. This assumes that `this` is always
Copy link
Collaborator

@CryZe CryZe May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I started using FIXME as long term things that need to be / should be fixed and TODOs being temporary self reminders to do something as part of the ongoing PR. So unless you plan to fix this as part of this PR, this should stay a FIXME.

Copy link
Member Author

Choose a reason for hiding this comment

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

In that case, can we rename our existing TODOs to FIXMEs instead?

@CryZe
Copy link
Collaborator

CryZe commented May 12, 2020

If you edit splits that aren't open and download splits, it currently replaces the splits in your timer, despite you never "touching your timer". I don't think that's what we want. Honestly, I feel like we long term probably don't even want to have the leaderboard in the run editor. But that's outside the scope of this PR.

@wooferzfg
Copy link
Member Author

@CryZe Should we always persist the splits from the leaderboard to the database if you click "OK"? That seems like the least confusing user experience based on what you said.

@CryZe
Copy link
Collaborator

CryZe commented May 12, 2020

Yeah I think that may be fine? I'm honestly not sure.

@wooferzfg
Copy link
Member Author

Also, I agree that the leaderboard shouldn't be in the Run Editor.

@wooferzfg wooferzfg requested a review from CryZe May 13, 2020 00:22
@wooferzfg wooferzfg force-pushed the leaderboard-splits branch 2 times, most recently from 9d80db8 to 1dee3a6 Compare May 13, 2020 14:03
@wooferzfg wooferzfg added bug There's a bug in LiveSplit One. UI The issue is about the user interface. labels May 17, 2020
@wooferzfg wooferzfg force-pushed the leaderboard-splits branch 3 times, most recently from a4cac16 to 4545bb6 Compare May 28, 2020 04:32
@wooferzfg wooferzfg force-pushed the leaderboard-splits branch 3 times, most recently from fe93f26 to 9473772 Compare June 14, 2020 17:01
@wooferzfg
Copy link
Member Author

@CryZe Can we merge this? I don't think there are remaining issues with this, at least as a fix for #362.

@CryZe
Copy link
Collaborator

CryZe commented Jan 18, 2021

I can re-review this tomorrow maybe.

@wooferzfg wooferzfg marked this pull request as draft April 16, 2024 17:52
@wooferzfg wooferzfg marked this pull request as ready for review April 17, 2024 00:53
@wooferzfg
Copy link
Member Author

@CryZe Could you take a look at the way I'm handling the run editor in this PR? Not sure if I'm disposing it properly everywhere

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CryZe CryZe Awaiting requested review from CryZe

Assignees
No one assigned
Labels
bug There's a bug in LiveSplit One. UI The issue is about the user interface.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Downloading splits from the Leaderboard should act as new splits
2 participants
@wooferzfg @CryZe