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

Remove country from language selection in the UI #7871

Closed
NateWr opened this issue Apr 18, 2022 · 12 comments
Closed

Remove country from language selection in the UI #7871

NateWr opened this issue Apr 18, 2022 · 12 comments
Assignees
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. Try Me This issue might be good for a new contributor. Can you help us?
Milestone

Comments

@NateWr
Copy link
Contributor

NateWr commented Apr 18, 2022

Describe the problem you would like to solve
There are a few places in the UI where the user can select to switch languages. These often show the full language designation, such as "French (Canadian)". However, it's very uncommon for two variants of the same language with the same script to be enabled at once, such as "French (Francais), French (Canadian)".

As a result, the country text is usually unnecessary, sometimes inappropriate, and can lead to a cluttered UI in some cases.

Describe the solution you'd like
Remove the country designation from some places where it isn't needed, such as:

  • Toggling the visible locales in forms.
  • The language block plugin where users switch the active locale
  • The user navigation menu in the backend where users switch the active locale

The country should remain in the language description where locale settings are managed (Settings > Website > Setup > Languages).

Who is asking for this feature?
This will make the UI a little bit cleaner, especially with forms. An informal survey was done in our general slack channel and there were no known cases of journals that needed two variants of the same language in the same script.

The only known cases were where journals included both to be more inclusive, because it looked odd or inappropriate to say "Portugese (Brazil)" when it also accepted contributions from Portugal (or any variant of the language).

Additional information
Here is an example of where the country information is inappropriate, since no other English variant is available in OJS but many journals don't want to use "American" english.

locales
locales-simple

@NateWr NateWr added Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. Try Me This issue might be good for a new contributor. Can you help us? labels Apr 18, 2022
@NateWr NateWr added this to the 3.4 milestone Apr 18, 2022
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 12, 2022
This commit removes the country name from locale names
whenever they are displayed to an end user. The exception
is when locales are listed in language installation forms,
where site admins or journal managers need to be able to
enable/disable different variants of the same language.
@NateWr
Copy link
Contributor Author

NateWr commented May 12, 2022

@jonasraoni can you take a look at this and let me know if this seems like an ok approach?

PR:
#7926
pkp/ui-library#201
pkp/ojs#3401
pkp/omp#1118
pkp/ops#279

@NateWr NateWr moved this to Backlog in Editorial Workflow May 12, 2022
@NateWr NateWr moved this from Backlog to Under Development in Editorial Workflow May 12, 2022
NateWr added a commit to NateWr/pkp-lib that referenced this issue May 12, 2022
NateWr added a commit to NateWr/ui-library that referenced this issue May 12, 2022
@NateWr
Copy link
Contributor Author

NateWr commented May 25, 2022

@jonasraoni or @asmecher do I need to do something special to support .po files in the unit tests? I'm getting a failed locale key in the tests here:

https://app.travis-ci.com/github/pkp/ojs/jobs/570021215#L4954-L4968

From this code:

https://github.com/pkp/pkp-lib/pull/7926/files#diff-ac2824060c1de3984e128d831df92701ed5115ad33b2cc39889a6958ba9ead1aR67-R73

But I can see that locale key exists in the pkp-lib committed to that repo:

4466bf1...9f2be9e#diff-2b17bf8f47344c18449a502cf760b03c71d8188477a98b225fcef6b3eb43a3a9

@jonasraoni
Copy link
Contributor

@NateWr I had a similar issue at the unit tests, here's the culprit: https://github.com/pkp/pkp-lib/blob/main/tests/mock/env1/MockLocale.inc.php#L35-L41

I just can't remember if I spent time analyzing the idea/objective (if the idea is just to overwrite translations, then it's not needed anymore, the Laravel's Facade can do it).

@NateWr
Copy link
Contributor Author

NateWr commented May 26, 2022

Thanks @jonasraoni. What would be the appropriate fix for the tests in my case? It doesn't look like MockLocale::setTranslations() is ever used...

@jonasraoni
Copy link
Contributor

Given that facades have mock capabilities out of the box, I think this class can be wiped out.

As a fast fix, I believe that removing the get() method and its related code will be enough (I found few occurrences of octothorpes inside the /tests folder to be updated). 🤞

@NateWr
Copy link
Contributor Author

NateWr commented May 26, 2022

I'm sorry I don't know what to do with this. @asmecher can you pick this up and get it merged? I'm not sure what needs to happen with the tests.

asmecher pushed a commit to asmecher/pkp-lib that referenced this issue Jun 7, 2022
This commit removes the country name from locale names
whenever they are displayed to an end user. The exception
is when locales are listed in language installation forms,
where site admins or journal managers need to be able to
enable/disable different variants of the same language.
asmecher pushed a commit to asmecher/pkp-lib that referenced this issue Jun 7, 2022
asmecher pushed a commit to asmecher/ui-library that referenced this issue Jun 7, 2022
asmecher added a commit to asmecher/pkp-lib that referenced this issue Jun 7, 2022
@asmecher
Copy link
Member

asmecher commented Jun 7, 2022

Just while I have a moment, testing to see whether removing get() fixes it --> pkp/ojs#3420
(This is the kind of thing we should not be testing outselves as we adopt more and more of the Laravel platform.)

edit: nope

@NateWr
Copy link
Contributor Author

NateWr commented Jun 22, 2022

In the interests of moving this along, can I remove the following lines from the unit test and file a separate issue about the locale problems in unit tests?

@jonasraoni
Copy link
Contributor

@NateWr I've replaced the old Mocks locally and did other cosmetic updates to the tests, I'll create a PR later to see if it passes through Travis 😂

@jonasraoni jonasraoni mentioned this issue Jun 27, 2022
8 tasks
@jonasraoni
Copy link
Contributor

As we've discussed, I'll handle the failing test in another issue, so you can skip the test by adding this to the beginning of the function:

$this->markTestSkipped('TODO: Will be fixed by the issue #8040');

NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2022
This commit removes the country name from locale names
whenever they are displayed to an end user. The exception
is when locales are listed in language installation forms,
where site admins or journal managers need to be able to
enable/disable different variants of the same language.
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2022
NateWr added a commit to NateWr/ui-library that referenced this issue Jun 28, 2022
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2022
NateWr added a commit to NateWr/pkp-lib that referenced this issue Jun 28, 2022
NateWr added a commit that referenced this issue Jun 29, 2022
NateWr added a commit to pkp/ui-library that referenced this issue Jun 29, 2022
pkp/pkp-lib#7871 Update test data after removing country from locale …
NateWr added a commit to pkp/ojs that referenced this issue Jun 29, 2022
NateWr added a commit to pkp/omp that referenced this issue Jun 29, 2022
pkp/pkp-lib#7871 Update tests after removing country from locale names
NateWr added a commit to pkp/ops that referenced this issue Jun 29, 2022
pkp/pkp-lib#7871 Update tests after removing country from locale names
@NateWr
Copy link
Contributor Author

NateWr commented Jun 29, 2022

At long last! I have journeyed to the top of Mount Testing and I have planted my ✔️ flag at its summit.

(Merged to main. Thanks @jonasraoni!)

@NateWr NateWr closed this as completed Jun 29, 2022
Repository owner moved this from Under Development to Done in Editorial Workflow Jun 29, 2022
@asmecher
Copy link
Member

asmecher commented Sep 3, 2022

The design choices made here are being revised again over here: #7352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement:1:Minor A new feature or improvement that can be implemented in less than 3 days. Try Me This issue might be good for a new contributor. Can you help us?
Projects
Status: Done
Development

No branches or pull requests

4 participants