-
-
Notifications
You must be signed in to change notification settings - Fork 538
[UX] Alt action to launch games with and without logs #4220
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
Conversation
| src={art_background || art_cover} | ||
| className="backgroundImage" | ||
| <SettingsContext.Provider value={settingsContextValues}> | ||
| <div className="gameConfigContainer"> |
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.
the diff here is a mess, but I'm just wrapping things in a SettingsContext wrapper, nothing changed inside this
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.
The "Hide whitespace" checkbox in the settings of GitHub's diff view is helpful here
| max-width: 560px; | ||
| } | ||
|
|
||
| & .playButtons { |
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.
this CSS is partially duplicated on purpose for both old and new design, eventually we'll remove the old styles and it's easier to do that with duplications
5494279 to
6a5e903
Compare
6a5e903 to
c8908de
Compare
src/backend/config.ts
Outdated
| afterLaunchScriptPath: '', | ||
| disableUMU: false, | ||
| verboseLogs: false | ||
| verboseLogs: true |
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.
enabled by default, since they can be disabled+play at the same time with a single action now
but by default we can get more logs for new users
12c07e5 to
83adbef
Compare
CommandMC
left a comment
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.
A couple of nitpicks
The feature itself sounds & looks good to me!
| function showAltPlayAction() { | ||
| if ( | ||
| is.syncing || | ||
| is.installingRedist || | ||
| is.installingWinetricksPackages || | ||
| is.launching || | ||
| is.playing | ||
| ) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| function getAltPlayLabel() { | ||
| if ( | ||
| is.syncing || | ||
| is.installingRedist || | ||
| is.installingWinetricksPackages || | ||
| is.launching || | ||
| is.playing | ||
| ) { | ||
| return <></> | ||
| } | ||
|
|
||
| if (verboseLogs) { | ||
| return ( | ||
| <span className="buttonWithIcon"> | ||
| <PlayArrow data-icon="play" /> | ||
| {t('label.playing.start')} | ||
| </span> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <span className="buttonWithIcon"> | ||
| <PlayArrow data-icon="play" /> | ||
| {t('label.playing.start_with_logs', 'Play Now (with logs)')} | ||
| </span> | ||
| ) | ||
| } | ||
|
|
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.
Consider making these memoized values to avoid computing them every re-render
| is.syncing || | ||
| is.installingRedist || | ||
| is.installingWinetricksPackages || | ||
| is.launching || | ||
| is.playing |
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.
Maybe factor this out into its own variable? Seems like this condition (although with some variation on which is values are considered) is used 3 times now
| disabled={ | ||
| is.reparing || | ||
| is.moving || | ||
| is.updating || | ||
| is.uninstalling || | ||
| is.syncing || | ||
| is.launching || | ||
| is.installingWinetricksPackages || | ||
| is.installingRedist | ||
| } | ||
| autoFocus={true} | ||
| onClick={async () => handlePlay(gameInfo)} |
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.
These moved from before the className prop to after it. Not a problem, just wanted to mention it since it seems unintentional (nothing apart from the position was changed)
| src={art_background || art_cover} | ||
| className="backgroundImage" | ||
| <SettingsContext.Provider value={settingsContextValues}> | ||
| <div className="gameConfigContainer"> |
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.
The "Hide whitespace" checkbox in the settings of GitHub's diff view is helpful here
8d034af to
d642add
Compare
flavioislima
left a comment
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.
Tested here, looks good.
This PR adds an alternative "Play Now (with logs)" action right next to the normal
Play Nowbutton in the details page behind a small dropdown.The idea is that users can easily select to launch a game with and without logs enabled for debugging purposes (and it's easier to instruct to do that and easier to discover).
The main action will be the last one used so users can clearly see if they are running the game with/without logs as a reminder that they can disable them, and both actions will change the game's settings so it's persisted (this way it can be used for things like running as a non-steam game and log things at the same time).
This depends on my previous PR making the logs configurable in a game-by-game basis.
play-logs.mp4
Use the following Checklist if you have changed something on the Backend or Frontend: