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] Delete duplicate ThemeSelector element #1580

Merged

Conversation

redromnon
Copy link
Collaborator

I found that ThemeSelector is present in both Accessibility and General Settings screens. Removed the extra one from General Settings.

I can remove it from the Accessibility screen instead.


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)

- ThemeSelector is present in both Accessibility and General Settings
- Removed the extra one from General Settings
@redromnon redromnon added the pr:ready-for-review Feature-complete, ready for the grind! :P label Jul 16, 2022
@arielj
Copy link
Collaborator

arielj commented Jul 17, 2022

Hmmm I added it in the Accessibility screen without removing it from General Settings because I didn't know where people would look for it:

  • people with accessibility needs may look for it in that screen to change colors to a theme that works better for them
  • people just looking for a visual change without any accessibility need may expect it in the general settings

I intentiontionally left it in both places.

It can be removed if people think it's useless to have it in both places, but I thought it made sense in both places (like we currently show the language selector in both the login screen and the settings)

@redromnon
Copy link
Collaborator Author

It perfectly makes sense to place the Language Selector on the Login screen too. As for the Theme selector is concerned, I feel one should be sufficient. But then again, as you mentioned, let's see what the others have to say about this.

@arielj
Copy link
Collaborator

arielj commented Jul 17, 2022

It perfectly makes sense to place the Language Selector on the Login screen too. As for the Theme selector is concerned, I feel one should be sufficient. But then again, as you mentioned, let's see what the others have to say about this.

maybe ask this in the UX-UI channel in discord for more visibility? since it's related to user experience

@flavioislima
Copy link
Member

I think it's fine to have it in two different places like it is today. But if we want to remove one, perhaps the one from settings would be better since we should make it easier to be found for people with disabilities.

@flavioislima flavioislima merged commit bf0e2c4 into Heroic-Games-Launcher:main Jul 17, 2022
@redromnon redromnon deleted the fix-duplicate-ThemeSelector branch July 18, 2022 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants