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] Add accessibility option to show all game tiles in color #1311

Merged

Conversation

connercsbn
Copy link
Contributor

@connercsbn connercsbn commented May 15, 2022

Patch to address feature request #1301

This adds an option in accessibility settings that, when enabled, adds the allTilesInColor class to the GameCard component, making all tiles show in color.

image
image

Just wanted to get this out there and get some feedback. Will do checklist soon!


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)

setAllTilesInColor(!allTilesInColor)
}}
title={t(
'accessibility.allTilesInColor',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use camel_case here accessibility.all_titles_in_color.

Also, please run yarn i18next after changing that so it updates all the locales for all languages.

@arielj arielj linked an issue May 15, 2022 that may be closed by this pull request
@connercsbn
Copy link
Contributor Author

Did something go wrong when I ran yarn i18next?

"library": {
"gridView": "نمای شبکه‌ای",
"gridView": "نمای شبکه\u200cای",
Copy link
Member

Choose a reason for hiding this comment

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

Could you remove this \u200c manually? this is i18n bug with languages like this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 3842ba3

@@ -66,14 +64,6 @@
"moving": "Преместване",
"repairing": "Поправяне"
},
"info": {
Copy link
Member

Choose a reason for hiding this comment

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

those removals doesnt seems right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's what I was thinking, but that's what happens when I run yarn i18next, even on the up-to-date main branch

Copy link
Member

Choose a reason for hiding this comment

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

yarn i18next or yarn i18n? should be the same, I guess. But I will try to investigate this later.

@flavioislima
Copy link
Member

Need to check why so many translations were deleted on this PR. I cannot find any change related to that, so we need to check main and run the command there to see whats happening because it is deleting a lot of keys from settings, gamepage, etc.

@flavioislima
Copy link
Member

flavioislima commented May 16, 2022

@arielj I believe that the missing keys are related to one of the refactors on the UI.
Some keys moved to a different file and then lost the translation.
Example: status.moving was on the gamepage.json but moved to translations.json and lost the value.
Maybe was when some components were moved. I dont know if this is breaking some language right now, but might be.
We should investigate that in another PR and try to move the keys around files, or maybe start using one single file for all translations.

@arielj
Copy link
Collaborator

arielj commented May 16, 2022

Agree on using one single file for all the translations, since we don't really need to modify those files manually anyway, should simplify some code and allow re-usability

@flavioislima
Copy link
Member

@arielj yes, that will be a lot of work, we have 64 translations file now, but will work best and it is better than losing all translations.

@flavioislima
Copy link
Member

@connercsbn @arielj
Found the issue and it is simple to solve.
Just edit this line:

const { t: t2 } = useTranslation('translation')

to put it like this:
const { t: t2 } = useTranslation()

and then run yarn i18n again. In case it doesnt fix it, try removing the folder locales and then run the command again. Should be fine.

The thing is that for the default file, that is translations.json we dont need to pass an argument, because it is the default.
I mean, the i18next should be able to catch that but it doesnt.

@flavioislima flavioislima added the pr:testing This PR is in testing, don't merge. label May 18, 2022
@@ -282,7 +286,6 @@
"title": "Manual Sync Saves",
"upload": "Upload"
},
"maxRecentGames": "Played Recently to Show",
Copy link
Member

Choose a reason for hiding this comment

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

weird that it is still removing a few keys not related with the PR. 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, looks like it's caused by a couple deleted lines from here: 421ae61

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you are right. Let me try to fix those

@flavioislima flavioislima changed the title add accessibility option to show all game tiles in color [UI] Add accessibility option to show all game tiles in color May 18, 2022
@flavioislima
Copy link
Member

ok, fixed it in main. Try to update your branch now.
also tested the feature and it is working fine. :)

Copy link
Contributor Author

@connercsbn connercsbn left a comment

Choose a reason for hiding this comment

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

Seems like last thing I need to do is revert the deletion of these lines, since they're being generated in English for these translation files. I'll see if I can grab the files from the main branch and rerun i18n

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.

Perfect! Thanks a lot for that! :D

@flavioislima flavioislima added pr:ready-to-merge This PR is fully ready for merge. and removed pr:testing This PR is in testing, don't merge. labels May 18, 2022
@flavioislima flavioislima merged commit b738081 into Heroic-Games-Launcher:main May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-to-merge This PR is fully ready for merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[UI] option to show all game tiles in color instead of black and white
4 participants