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

feat/add context menu #236

Closed
wants to merge 11 commits into from
Closed

feat/add context menu #236

wants to merge 11 commits into from

Conversation

guibzo
Copy link
Contributor

@guibzo guibzo commented May 8, 2024 •
edited
Loading

image

  1. Open archive location --> if user has already selected an exe it opens the folder based on this exe location. If not, it opens the installation folder path.
  2. Change executable path --> Self-explained.
  3. Remove installation archives --> Deletes installation folder; the one located on the download path. Keeps other files (such as the folder where the selected exe with the installed game is) intact. Helpful to get rid of unused archives and free disk space. (includes dialog to confirm if user wants to proceed with action).

solve #220
solve #130
solve #118
solve #71
solve #416
solve #484

partial solve #421
partial solve #468

lilezek
lilezek suggested changes May 9, 2024
View reviewed changes
src/main/events/library/open-game-folder.ts
Comment on lines +18 to +24
if (game.executablePath) {
gamePath = path.dirname(game.executablePath);
} else {
gamePath = game.folderName
? path.join(await getDownloadsPath(), game.folderName)
: await getDownloadsPath();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This opens either the installer folder or the game folder, but I think this may be confusing. The installer's folder and the executable's path should be something different. We could disable the option in the context menu if the path is missing.

Copy link
Contributor Author

@guibzo guibzo May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since games can have a "done installation" folder (where the .exe is located at) and a "to install" folder (where the default installation path is set at configs) it would be a better experience to always have something addressed to "Open archive location" button, if there is any archives at all.

src/renderer/src/components/sidebar/sidebar.tsx Outdated Show resolved Hide resolved
src/renderer/src/components/sidebar/sidebar.tsx Outdated Show resolved Hide resolved
src/renderer/src/components/sidebar/sidebar.tsx Outdated Show resolved Hide resolved
src/renderer/src/hooks/use-download.ts Outdated Show resolved Hide resolved
src/renderer/src/pages/game-details/hero/hero-panel-actions.tsx Outdated Show resolved Hide resolved
@guibzo
Copy link
Contributor Author

guibzo commented May 9, 2024

adjusted

@guibzo guibzo force-pushed the feat/add-context-menu branch from 542307e to 2666d48 Compare May 9, 2024 17:48
@lilezek
Copy link
Contributor

lilezek commented May 9, 2024

adjusted

Everything looks good but the transformation from <ul> to <div>. Can you leave it to be an <ul> as it was originally?

Also, I'm still wondering why is updating the library needed before opening a game. I'm not saying it is wrong, but I don't get it. Could you elaborate?

@guibzo
Copy link
Contributor Author

guibzo commented May 9, 2024

adjusted

Everything looks good but the transformation from <ul> to <div>. Can you leave it to be an <ul> as it was originally?

Also, I'm still wondering why is updating the library needed before opening a game. I'm not saying it is wrong, but I don't get it. Could you elaborate?

debugging reasons.
changed already

@lilezek
Copy link
Contributor

lilezek commented May 9, 2024

LGTM

@guibzo guibzo force-pushed the feat/add-context-menu branch from aa6be6b to 4fdc37f Compare May 23, 2024 12:22
@guibzo
Copy link
Contributor Author

guibzo commented May 23, 2024 •
edited
Loading

review/merge please
@thegrannychaseroperation
@zamitto

zamitto
zamitto reviewed May 27, 2024
View reviewed changes
src/main/entity/game.entity.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have changes on this file. Keep as little changes as possible, please

zamitto
zamitto reviewed May 27, 2024
View reviewed changes
src/main/events/misc/show-open-dialog.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have changes on this file. Keep as little changes as possible, please.

And AFAIK, the function used in the event needs to be asyn

zamitto
zamitto reviewed May 27, 2024
View reviewed changes
src/main/events/library/open-game.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have changes on this file. Keep as little changes as possible, please

zamitto
zamitto reviewed May 27, 2024
View reviewed changes
src/renderer/src/assets/discord-icon.svg Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Discord icon is not used anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should sub for telegram?

zamitto
zamitto reviewed May 27, 2024
View reviewed changes
src/main/events/library/delete-game-folder.ts Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to have changes on this file. Keep as little changes as possible, please.

@zamitto
Copy link
Collaborator

zamitto commented May 27, 2024

Are you using Prettier with the project settings? A few changes seems that would be fixed by prettier. And overall, try to make as little changes as possible. I made just a quick review

@guibzo
Copy link
Contributor Author

guibzo commented May 28, 2024 •
edited
Loading

Are you using Prettier with the project settings? A few changes seems that would be fixed by prettier. And overall, try to make as little changes as possible. I made just a quick review

I am using the prettier.

@guibzo
Copy link
Contributor Author

guibzo commented May 28, 2024

adjusted

@guibzo guibzo force-pushed the feat/add-context-menu branch from b8d45e2 to 4a1967d Compare May 28, 2024 15:58
@guibzo guibzo requested a review from zamitto May 28, 2024 16:10
@guibzo guibzo closed this Jun 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lilezek lilezek lilezek requested changes

@zamitto zamitto Awaiting requested review from zamitto

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

Successfully merging this pull request may close these issues.
3 participants
@guibzo @lilezek @zamitto