docusaurus icon indicating copy to clipboard operation
docusaurus copied to clipboard

feat(theme-classic): show current locale in mobile language selector

Open massoudmaboudi opened this issue 2 years ago • 28 comments

Pre-flight checklist

  • [x] I have read the Contributing Guidelines on pull requests.
  • [x] If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
  • [ ] If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

fix #7404

massoudmaboudi avatar May 13 '22 03:05 massoudmaboudi

[V2]

Built without sensitive environment variables

Name Link
Latest commit f9aea8a4e4b50f32f9a33438ed3d5148743312a3
Latest deploy log https://app.netlify.com/sites/docusaurus-2/deploys/628dccb2c1b47d00081c13e4
Deploy Preview https://deploy-preview-7409--docusaurus-2.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar May 13 '22 03:05 netlify[bot]

⚡️ Lighthouse report for the deploy preview of this PR

URL Performance Accessibility Best Practices SEO PWA Report
/ 🟠 80 🟢 100 🟢 100 🟢 100 🟢 90 Report
/docs/installation 🟠 84 🟢 99 🟢 100 🟢 100 🟢 90 Report

github-actions[bot] avatar May 13 '22 03:05 github-actions[bot]

See also https://github.com/facebook/docusaurus/pull/7430

Josh-Cena avatar May 16 '22 08:05 Josh-Cena

#7430 is a good workaround. but as we have the same issue (duplication of same labels) in the desktop version also, I want to mention that as you noticed, the site maybe is Chinese or Farsi, but suddenly a Languages word appears there which is not sweet.

I have another suggestion which I want to know your opinion also. We can remove the current locale from the drop-down locale menu. Any time a new locale is selected, we can remove only the selected locale and show the rest of the list.

I don't like my suggestion honestly and I prefer to see duplicate labels still with the current locale duplicated.

massoudmaboudi avatar May 17 '22 04:05 massoudmaboudi

What about adding a (current) text after the current locale in the dropdown?

Josh-Cena avatar May 17 '22 04:05 Josh-Cena

What about adding a (current) text after the current locale in the dropdown?

Too long and again there will be an English word. highlighting works here. I just dont like the locale dropdown label

massoudmaboudi avatar May 17 '22 05:05 massoudmaboudi

Too long and again there will be an English word.

If you translate your theme, neither "current" nor "languages" will be English.

I just dont like the locale dropdown label

We have the same UX for the versions dropdown as well. If anything, we need to sync the two places. I don't know why we had it in the first place, though.

Josh-Cena avatar May 17 '22 05:05 Josh-Cena

Too long and again there will be an English word.

If you translate your theme, neither "current" nor "languages" will be English.

I just dont like the locale dropdown label

We have the same UX for the versions dropdown as well. If anything, we need to sync the two places. I don't know why we had it in the first place, though.

so what's your final suggestion, Josh?

massoudmaboudi avatar May 17 '22 05:05 massoudmaboudi

I don't really know😅 I'll be holding my opinion and let @slorber make the call.

In the meantime, you can always swizzle your own component. It's a pretty minor change.

Josh-Cena avatar May 17 '22 05:05 Josh-Cena

I don't really know😅 I'll be holding my opinion and let @slorber make the call.

In the meantime, you can always swizzle your own component. It's a pretty minor change.

yes yes, I did it already, but lets wait for him then. Thanks for all your hard work

massoudmaboudi avatar May 17 '22 05:05 massoudmaboudi

Apparently I introduced these different labels in mobile:

  • locale: https://github.com/facebook/docusaurus/pull/3916/files#diff-32ab16d58780568e2ae1086a1867a977d1768d7789819e5aa7aa0b8bc763695aR64
  • version: https://github.com/facebook/docusaurus/pull/3088/files#diff-01a51be5a9f804a89da8f3ced002fcf396b245f45fe45dd1acbc8dc81cab26a5R48

Can't remember exactly what was the motivation, but this UX looks good to me 🤷‍♂️

I don't mind changing it, but the best UX is quite subjective: I'd suggest to poll our user-base on this first before deciding if it's worth pursuing. It's not because you like it @massoudmaboudi that others will do to.

At worst (if community disagree), we could make it easier to swizzle the dropdown label so that you can render it the same on desktop/mobile

slorber avatar May 25 '22 17:05 slorber

Created a poll to ask our community:

  • Twitter here: https://twitter.com/sebastienlorber/status/1529513303344570368
  • Discord here: https://discord.com/channels/398180168688074762/867060988717301780/979072824125845526

We'll see what users prefer

slorber avatar May 25 '22 17:05 slorber

@massoudmaboudi I believe there is a misunderstanding here, after reading your tweet: https://twitter.com/MaboudiMassoud/status/1529765739053580289

CleanShot 2022-05-26 at 14 36 48@2x

Of course, it does not look very good to have a "Languages" label in English, when the site is in Farsi.

The only reason it's not translated is that... you forgot to translate it here: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-translations/locales/fa/theme-common.json#L55

The real question is not "Languages" vs "فارسی"

But rather: "زبان ها" vs "فارسی"

Does it make more sense?

slorber avatar May 26 '22 12:05 slorber

It looks like there's no clear winner in the poll atm:

CleanShot 2022-05-26 at 14 43 35@2x CleanShot 2022-05-26 at 14 44 16@2x

Some facts:

  • The poll is kind of a draw for now
  • People may misinterpret 1) as "hardcoded English label" (like you probably did, while it is localized) and bias toward 2)
  • We already have translations for 1)

Some arguments to keep 1)

CleanShot 2022-05-26 at 14 47 17@2x

Another arg that make sense and confirms that having different label/ux for desktop/mobile looks better:

CleanShot 2022-05-26 at 14 47 34@2x

Considering this, I'd rather keep using 1) as the default implementation.

If we allow 2, that would either be with swizzle or with an opt-in option.

Using 2) as default would be more complicated for users to move to 1), particularly if we drop the existing localized label by default.


Should we close this PR, or do you want to update it and make it easier to enable 2) ?

slorber avatar May 26 '22 12:05 slorber

@massoudmaboudi I believe there is a misunderstanding here, after reading your tweet: https://twitter.com/MaboudiMassoud/status/1529765739053580289

CleanShot 2022-05-26 at 14 36 48@2x

Of course, it does not look very good to have a "Languages" label in English, when the site is in Farsi.

The only reason it's not translated is that... you forgot to translate it here: https://github.com/facebook/docusaurus/blob/main/packages/docusaurus-theme-translations/locales/fa/theme-common.json#L55

The real question is not "Languages" vs "فارسی"

But rather: "زبان ها" vs "فارسی"

Does it make more sense?

You are right, that's what I explained in the reply to my tweet. There is a third option to keep Versions as it is, but change Languages to English

massoudmaboudi avatar May 26 '22 13:05 massoudmaboudi

I prefer to add an option to select label or item for each dropdown, both language or version.

But I should read your technical implementation on how to add it in docusarous.config.js

massoudmaboudi avatar May 26 '22 13:05 massoudmaboudi

I prefer to add an option to select label or item for each dropdown, both language or version.

Why not then 👍

How do we design this API?

Note if this option only applies to the mobile drawer, we should make it clear it only applies on mobile.

Eventually, providing 2 options for flexibility and consistency?

For example:

{
  type: 'docsVersionDropdown',
  mobileLabelType: "static" | "dynamic",
  desktopLabelType: "static" | "dynamic",
}

Any opinion on this API @Josh-Cena @massoudmaboudi ?

Do you want to implement it yourself?

slorber avatar May 26 '22 14:05 slorber

There's quite the same idea in https://github.com/facebook/docusaurus/issues/5415. I'm personally in favor of it.

Josh-Cena avatar May 26 '22 14:05 Josh-Cena

There's quite the same idea in https://github.com/facebook/docusaurus/issues/5415. I'm personally in favor of it.

But to solve this a different design would be needed as you need as the current one use a union type while the other issue probably needs more freedom to pass arbitrary labels: "static" | "dynamic" | string ? (+ localize the arbitrary label?)

slorber avatar May 26 '22 14:05 slorber

😵‍💫

Josh-Cena avatar May 26 '22 14:05 Josh-Cena

Oh I get what you mean. Yeah yeah, what about label: "current-choice" | { text: string }? We can default to { text: "Languages" }. This does mean we lose default theme translations, though.

Josh-Cena avatar May 26 '22 14:05 Josh-Cena

This does mean we lose default theme translations

That's also why I'd prefer to have a 2nd enum string value: to keep it by default. But I'm not a fan to mix string enums with string type, as somehow it implies that some values are "magical"

slorber avatar May 26 '22 14:05 slorber

Right, hence "current-choice" | { text: string }... Too bad symbols aren't serializable, else I'll suggest Symbol.for("current-choice") 😄 Another choice would be false | undefined | string where undefined is the default label, false is the current choice. A bit magical as well.

Josh-Cena avatar May 26 '22 14:05 Josh-Cena

mobileLabelType: "languages" | "current-choice" | { text: string } ?

slorber avatar May 26 '22 14:05 slorber

What about using undefined in place of "languages"? Does it have to be explicitly specifiable?

Josh-Cena avatar May 26 '22 14:05 Josh-Cena

Honestly, I like the option to change in both mobile and desktop. For both version and locale.

But I doubt my implementation be as professional as yours since I am a backend developer :)

massoudmaboudi avatar May 26 '22 15:05 massoudmaboudi

mobileLabelType: undefined | "current-choice" | { text: string } => defaults to undefined
desktopLabelType: undefined | "current-choice" | { text: string } => defaults to "current-choice"

Is this good enough?

Does it have to be explicitly specifiable?

For Desktop, which is using "current-choice" by default (ie default value), if the user wants to display the "static" label instead, I'm not sure passing "undefined" is a good idea, nor that it will play well with Joi default value that will keep being applied.

But I doubt my implementation be as professional as yours since I am a backend developer :)

No problem, just tell us to work on it if you won't

slorber avatar May 26 '22 17:05 slorber

Please take care of this feature request like the others :)

massoudmaboudi avatar May 27 '22 01:05 massoudmaboudi