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

Add cancel button for exiting install modal #835

Merged
merged 11 commits into from
Jan 3, 2022
Merged

Add cancel button for exiting install modal #835

merged 11 commits into from
Jan 3, 2022

Conversation

Artumira96
Copy link
Contributor

Solves issue: #826 of adding the "Cancel" button for the install modal

  • Tested the feature and it's working on a current and clean install.

Here's a quick demo of the functionality:

Screen.Recording.2021-12-29.at.14.20.51.mov

Regards, Arturo

@ghost ghost requested review from flavioislima and a user December 29, 2021 22:34
ghost
ghost previously approved these changes Dec 29, 2021
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM, nice PR

flavioislima
flavioislima requested changes Dec 30, 2021
View reviewed changes
Copy link
Member

@flavioislima flavioislima left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi, thanks for the PR
I know it works fine but I think having three buttons there is not ideal. can you go with the Close button solution?
An X icon that you can grab from FontAwesome (I think we already have a use case in this component). The icon name is: faXmark.

Add some margin on it as well since that example has none.

@Artumira96
Copy link
Contributor Author

image

Got it, I changed the button to a close icon on the upper left corner, hope that makes the trick :) Here's a quick video of it:

Screen.Recording.2021-12-30.at.12.05.45.mov

@flavioislima
Copy link
Member

Nice, can you make it a big bigger and post a screenshot with it on the right corner as well, so we can decide which one is better?

@Artumira96
Copy link
Contributor Author

Sure, here's on the right a little bigger, what do you think?
image

On the left:
image

@flavioislima
Copy link
Member

Thanks. Let's go with smaller icon on the right. I think it's the best way 🙂

@Artumira96
Copy link
Contributor Author

You got it, it's on my last commit
image

arielj
arielj reviewed Dec 30, 2021
View reviewed changes
src/index.css Outdated Show resolved Hide resolved
arielj
arielj reviewed Dec 30, 2021
View reviewed changes
src/App.css Outdated Show resolved Hide resolved
arielj
arielj reviewed Dec 30, 2021
View reviewed changes
src/App.css Outdated Show resolved Hide resolved
ghost
ghost suggested changes Jan 1, 2022
View reviewed changes
.vscode/settings.json Outdated
"statusBarItem.hoverBackground": "#fa1b49",
"statusBarItem.remoteBackground": "#dd0531",
"statusBarItem.remoteForeground": "#e7e7e7",
"titleBar.activeBackground": "#dd0531",
Copy link

Choose a reason for hiding this comment

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

why are all this settings?

electron/main.ts Outdated
webviewTag: true,
contextIsolation: false,
Copy link

Choose a reason for hiding this comment

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

Don't change these

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah those 2 files weren't supposed to be in the commit, sorry about that

arielj
arielj reviewed Jan 1, 2022
View reviewed changes
Copy link
Collaborator

@arielj arielj left a comment

Choose a reason for hiding this comment

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

Apart from dragonDScript's comments about those extra changes I only have a question about the use of !important.

The rest looks good to me

src/screens/Library/components/InstallModal/index.css Outdated Show resolved Hide resolved
ghost
ghost reviewed Jan 1, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM; nice PR. fix what the others told you and it'll be good to go

@ghost ghost self-requested a review January 1, 2022 23:32
arielj
arielj approved these changes Jan 3, 2022
View reviewed changes
Copy link
Collaborator

@arielj arielj left a comment

Choose a reason for hiding this comment

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

great!

flavioislima
flavioislima approved these changes Jan 3, 2022
View reviewed changes
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.

LGTM 👍🏽
thanks for that!

@flavioislima flavioislima merged commit b14438a into Heroic-Games-Launcher:main Jan 3, 2022
@flavioislima flavioislima linked an issue Jan 3, 2022 that may be closed by this pull request
add an exit button. #826
Closed
@Artumira96
Copy link
Contributor Author

Glad I could help! :) thanks

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

@flavioislima flavioislima flavioislima approved these changes

@arielj arielj arielj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add an exit button.
4 participants
@Artumira96 @flavioislima @arielj @Artumira8