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

[Fix] Prevent empty name category from being added #3761

Conversation

DenysMb
Copy link
Contributor

@DenysMb DenysMb commented May 18, 2024

Currently we can add empty categories and also rename categories for an empty string.
This is just a small fix to prevent those cases.
I also did another one change to disable the "confirm rename button" when the name was not changed.

Before:

Before.mp4

After:

After.mp4

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)

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.

Sorry, one more thing, it would be good to update the e2e/categories.spec.ts to test these changes

it should be easy to update the test to check that the button is disabled with blank and unchanged values

@DenysMb
Copy link
Contributor Author

DenysMb commented May 22, 2024

Sorry, one more thing, it would be good to update the e2e/categories.spec.ts to test these changes

it should be easy to update the test to check that the button is disabled with blank and unchanged values

No worries. Everything is valid to increase the code quality.
I update the test, but I couldn't test. I receive the following error: Error: electron.launch: Process failed to launch!.
Probably something related to openSUSE Tumbleweed + Nvidia, I think... Because when I need to run the start script, I need to add app.commandLine?.appendSwitch('disable-gpu-sandbox') to main.ts to be able to run the application.
I'm trying to solve this, so I can check if is everything okay with the added scenarios.

Detailed error

This error happen for this four tests: renders the first page, gets heroic, legendary, and gog versions, categories and webview.

X) helpers.ts:19:7 > (one of these four tests - it happens with all of them, so is the same error repeated 4 times)

Error: electron.launch: Process failed to launch!

      18 | ) {
      19 |   test(name, async () => {
    > 20 |     const app = await electron.launch({
         |                 ^
      21 |       args: [main_js]
      22 |     })
      23 |     await func(app)

        at /storage/Projects/Contributions/HeroicGamesLauncher/e2e/helpers.ts:20:17

@@ -23,8 +26,20 @@ electronTest('categories', async (app) => {
dialog.locator('span', { hasText: 'Great games' })
).toBeInViewport()

// rename category
// add button should be disable if trying to rename category to empty name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you are testing that the button is disabled when the name is unchanged, the comment doesn't match that

the other 2 expects are for the blank tests

I can merge this after that's fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay in fixing this.
These previous days were very busy.
I fixed this now. Where it says "empty" should be "same", I didn't realize that "words swap".

@arielj
Copy link
Collaborator

arielj commented May 25, 2024

thanks! merging

@arielj arielj merged commit 8c77906 into Heroic-Games-Launcher:main May 25, 2024
9 checks passed
@Heroic-Games-Launcher Heroic-Games-Launcher locked and limited conversation to collaborators May 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants