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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

massoudmaboudi
Copy link
Contributor

@massoudmaboudi massoudmaboudi commented May 13, 2022

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • 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

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 13, 2022
@netlify
Copy link

netlify bot commented May 13, 2022

[V2]

Built without sensitive environment variables

Name Link
🔨 Latest commit f9aea8a
🔍 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.

@github-actions
Copy link

github-actions bot commented May 13, 2022

⚡️ 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

@Josh-Cena Josh-Cena changed the title feat(theme-classic): Mobile Version Language Toggle Should Show Current Locale feat(theme-classic): show current locale in mobile language selector May 14, 2022
@Josh-Cena Josh-Cena added the pr: polish This PR adds a very minor behavior improvement that users will enjoy. label May 16, 2022
Copy link
Collaborator

@Josh-Cena Josh-Cena left a comment

Choose a reason for hiding this comment

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

TBH, the UX is not great as I see it. The same label is repeated twice, which is what we are trying to avoid, both here and for the versions dropdown. What's lacking is the active link highlight color:

image

In addition to the gray background, we need to apply the primary color. The problem is that the dropdown__link--active is overridden by menu__link:

image

We need to use menu__link--active instead of dropdown__link--active on mobile, as we did for the regular navbar dropdowns (through the getInfimaClassName function)

@massoudmaboudi Do you think highlighting the current locale with the primary color is enough for you?

@Josh-Cena
Copy link
Collaborator

See also #7430

@massoudmaboudi
Copy link
Contributor Author

#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.

@Josh-Cena
Copy link
Collaborator

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

@massoudmaboudi
Copy link
Contributor Author

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

@Josh-Cena
Copy link
Collaborator

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.

@massoudmaboudi
Copy link
Contributor Author

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?

@Josh-Cena
Copy link
Collaborator

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.

@massoudmaboudi
Copy link
Contributor Author

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

@slorber
Copy link
Collaborator

slorber commented May 25, 2022

Apparently I introduced these different labels in mobile:

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
Copy link
Collaborator

slorber commented May 25, 2022

Created a poll to ask our community:

We'll see what users prefer

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

@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
Copy link
Collaborator

slorber commented May 26, 2022

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) ?

@massoudmaboudi
Copy link
Contributor Author

@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
Copy link
Contributor Author

massoudmaboudi commented May 26, 2022

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

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

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?

@Josh-Cena
Copy link
Collaborator

There's quite the same idea in #5415. I'm personally in favor of it.

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

There's quite the same idea in #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?)

@Josh-Cena
Copy link
Collaborator

😵‍💫

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented May 26, 2022

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.

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

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"

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented May 26, 2022

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.

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

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

@Josh-Cena
Copy link
Collaborator

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

@massoudmaboudi
Copy link
Contributor Author

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 :)

@slorber
Copy link
Collaborator

slorber commented May 26, 2022

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

@massoudmaboudi
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: polish This PR adds a very minor behavior improvement that users will enjoy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile Version Language Toggle should show current locale
4 participants