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.

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

[UI/UX] Replace native dialogs #1891

Merged
merged 25 commits into from Oct 20, 2022
Merged

[UI/UX] Replace native dialogs #1891

merged 25 commits into from Oct 20, 2022

Conversation

BrettCleary
Copy link
Collaborator

This refactors native error and message dialog boxes into modals.

The GlobalState ContextProvider now has showDialogModal and dialogModalOptions. Any frontend component can call showDialogModal to show a dialog. This is achieved with the DialogHandler component which uses dialogModalOptions to determine what dialog to show. The DialogHanlder also listens for dialogs that the backend requests to show.

Before:
Native Uninstall Modal
Native Reset Heroic
Native Cache Cleared

After:
Uninstall Modal
EAC runtime enabled modal
Cache Cleared Modal


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@BrettCleary BrettCleary linked an issue Oct 11, 2022 that may be closed by this pull request
@flavioislima
Copy link
Member

flavioislima commented Oct 11, 2022

so far is looking pretty good, just a few comments on the current state on Linux:
You can see on the video below that when uninstalling a windows game on linux it shows the uninstall dialog without the checkbox to delete the prefix and then the checkbox appears. So maybe we could delay the checkbox first or check if it is not a native game and is linux, then show the checkbox by default.

simplescreenrecorder-2022-10-11_14.24.53.mp4

And after clicking uninstall the dialog doesn't close until the action is complete, which makes Heroic hang for a few seconds. So perhaps adding or an indicator that the operation is ongoing or closing the dialog without waiting for the promise to be fulfilled. Other than that looks cool 👍🏽

@Nocccer Nocccer self-requested a review October 11, 2022 13:31
src/backend/api/misc.ts Outdated Show resolved Hide resolved
src/backend/main.ts Outdated Show resolved Hide resolved
src/backend/api/misc.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Nocccer Nocccer left a comment

Choose a reason for hiding this comment

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

Looks really promising. I left some suggestions i find useful.
Even if we even not have a single error dialog with actions at the moment, maybe we need one in the future where we provide actions on a error dialog, which mostly comes from the backend.

@BrettCleary
Copy link
Collaborator Author

Looks really promising. I left some suggestions i find useful. Even if we even not have a single error dialog with actions at the moment, maybe we need one in the future where we provide actions on a error dialog, which mostly comes from the backend.

Thanks. Appreciate the comments. Really helpful.

Yeah. Honestly the ErrorDialog and MessageBoxModal components are both very similar codewise. I think I'll combine them and pass the DialogType enum as a prop. That way we can support actions on error dialogs and don't have to duplicate code.

…uto fxn, refactor error dialog into message box modal, uninstall modal closes immediately on input
…button click, optimization for linux uninstall modal
@BrettCleary
Copy link
Collaborator Author

so far is looking pretty good, just a few comments on the current state on Linux: You can see on the video below that when uninstalling a windows game on linux it shows the uninstall dialog without the checkbox to delete the prefix and then the checkbox appears. So maybe we could delay the checkbox first or check if it is not a native game and is linux, then show the checkbox by default.

And after clicking uninstall the dialog doesn't close until the action is complete, which makes Heroic hang for a few seconds. So perhaps adding or an indicator that the operation is ongoing or closing the dialog without waiting for the promise to be fulfilled. Other than that looks cool 👍🏽

This is fixed now. Closes immediately. Also uses the context provider to get the platform instead of awaiting for it. Doesn't show the dialog until it checks this so no re-render now

@BrettCleary
Copy link
Collaborator Author

I am having an issue with absolute imports now though

[vite]: Rollup failed to resolve import "common/types" from "src/backend/main.ts".

Relative imports are working fine and it seems to only be common/types after I added a couple types to it.

@BrettCleary
Copy link
Collaborator Author

Okay so adding an enum in the common/types module breaks all the imports.

In troubleshooting this, I went deep into the rabbit hole regarding absolute imports and managed to get it working for the frontend and backend without the vite-tsconfig-paths plugin. Before, we could only use imports in the frontend.

The prefix alias is @@/ which maps to the src directory.

@BrettCleary BrettCleary marked this pull request as ready for review October 13, 2022 05:45
@BrettCleary
Copy link
Collaborator Author

This is working for me and tests are updated so I'll take it out of draft now

@BrettCleary
Copy link
Collaborator Author

I think we might be able to add backend common and frontend as aliases instead of @@ now too to keep the same style absolute imports everywhere. I'll test this

@flavioislima
Copy link
Member

flavioislima commented Oct 14, 2022

Testing here and now the Linux dialog for uninstalling looks good. Maybe just add a limit to the size of the dialogs in general, this one is huge, for instance:
image

@flavioislima flavioislima changed the title Feat/replace native dialogs [UI/UX] Replace native dialogs Oct 14, 2022
src/backend/utils.ts Outdated Show resolved Hide resolved
@BrettCleary
Copy link
Collaborator Author

Testing here and now the Linux dialog for uninstalling looks good. Maybe just add a limit to the size of the dialogs in general, this one is huge, for instance: image

done.

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

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

Looks great. Thanks for doing this ⚔️

@BrettCleary BrettCleary dismissed Nocccer’s stale review October 20, 2022 17:55

Comments have been addressed

@BrettCleary BrettCleary merged commit a03347e into beta Oct 20, 2022
@BrettCleary BrettCleary deleted the feat/replace_native_dialogs branch October 20, 2022 17:57
@biliesilva
Copy link

Testing here and now the Linux dialog for uninstalling looks good. Maybe just add a limit to the size of the dialogs in general, this one is huge, for instance: image

done.

It's important to differentiate the buttons No and YES, because this can confuse the user. The button that has more importance has to be all filled with color. Also is better to reduce the size of the box as you said.

@flavioislima
Copy link
Member

@biliesilva the size was fixed. But yes, that is a good point about the colors.
I think we will need to open a new Issue here on GH so we can fix that.

@BrettCleary
I think we can add a new prop to the dialog to choose which button is important or not.
Or maybe just highlight the first (or last) button by default with a different color.

@BrettCleary
Copy link
Collaborator Author

BrettCleary commented Oct 24, 2022

@biliesilva the size was fixed. But yes, that is a good point about the colors. I think we will need to open a new Issue here on GH so we can fix that.

@BrettCleary I think we can add a new prop to the dialog to choose which button is important or not. Or maybe just highlight the first (or last) button by default with a different color.

We can add a style enum or string type to the ButtonOptions type in src/common/types for buttonStyle and then have predefined css styles for each value applied in the DialogHandler component to the button

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[General UI/UX] Replace Native Dialogs with HTML Dialogs
6 participants