fix: redesign of language selector #2839

schalkneethling · 2021-02-07T14:25:29Z

Implement redesign of language selector and related interactions.
This also includes a change to how the underlying grid is implemented.

This addresses a number of issues as listed below. The main issue is the language selector though. The layout has also changed some as we move towards https://www.figma.com/file/YYVJ8uqG8UFBBvQhdI7fxR/MDN-Web-Docs?node-id=709%3A0

Also note, because of the complexity of having the language selector in the footer, it is now under the metadata section. The metadata section has also been moved to be under the "See also" section and is no longer in a full-width bar.

You will need a clone of the translated-content-rendered repo and the following setting in .env
CONTENT_TRANSLATED_ROOT=/Users/username/repos/translated-content-rendered/files

With this though, you still need to be on a non-English page to see the language toggle and language selector. I am not 100% but, I think this might not be the case on production and there the language selector and language will always be shown if translations are available.

Mobile

Tablet

Desktop

Desktop - no language toggle

Language selector

fix #1486
fix #2478
fix #2755

Implement redesign of language selector and related interactions. This also includes a change to how the underlying grid is implemented. Part of #1486

schalkneethling · 2021-02-08T13:31:06Z

Note: I am trying to understand what exactly is making the test above fail. If anyone has a hint, I am all ears :) I think it might have something to do with the language selector, but not 100%

schalkneethling · 2021-02-08T15:37:22Z

An interesting thing I notice is if I add console.log(await page.content()); to line 59 of testing/tests/headless.test.js and only run this test, the page title and h1 is:

\"title\":\"A Test Introduction to CSS Flexbox Layout\"}],\"pageTitle\":\"A Test Introduction to CSS Flexbox Layout | MDN\",

So it seems like it is loading testing/content/files/en-us/learn/css/css_layout/introduction/flex/index.html instead of testing/content/files/en-us/learn/css/css_layout/introduction/index.html

That seems misleading though because, even if I change the lines 59-62 to the following:

await expect(page).toMatch("A Test Introduction to CSS Flexbox layout");
    await expect(page).toMatchElement("h1", {
      text: "A Test Introduction to CSS Flexbox layout",
    });

the test still fails

peterbe

Those headless tests are weird!! I still can't explain how it could be that you're suddenly on that "Flex" page. However, if you run:

TESTING_OPEN_BROWSER=true yarn test:testing headless

you'll notice the same strange thing as you see in responsive mode
when you visit http://localhost:5000/en-US/docs/Learn/CSS/CSS_layout/Introduction/

I don't know why puppeteer is having a problem with that page and why it doesn't happen in a regular browser. But if you scroll down you'll see that the page is very messed up and perhaps that's causing something that expect-puppeteer can't handle. It's like it's freaking out. After all, the toMatch() function isn't trivial. It's based on finding things you can find with Cmd-f.

There are many other things that I think we should discuss in this PR.
We should, first of all, remove all the unrelated changes that slipped in that have nothing to do with the language selector. They deserve their own PRs with discussions.

The thing I don't like is that the "document footer" has lost its styling. We worked hard to come to the conclusion and design that the document footer would look different and "stand out" more from the last couple of paragraphs of text in the document:

after

*before*

Basically, I prefer the way it looked before. It made it more obvious that this was functionality stuff and not part of the article text.

The other thing I don't like is that switching to English doesn't set the cookie. It's an imperfect thing that changing language with the drop-down actually sets a persistent cookie but it's so very convenient. And it's understandable because you're very deliberately doing something. Simply clicking a link is questionable if it's very deliberate. But after all, it's part of the "change language" functionality and I think I would be disappointed if I have to click English a second time when I've already pressed it once.

client/src/document/organisms/toc/index.tsx

client/src/ui/atoms/page-content/index.tsx

client/src/ui/molecules/breadcrumbs/index.scss

client/src/ui/molecules/language-toggle/index.tsx

peterbe · 2021-02-08T18:54:50Z

Another thing I don't understand is; Why is the "Change language" button an outline one?
The teal-colored buttons we have (Prev and Next buttons for example) don't use outline.
I've never got the hang on outlined vs. not-outlined buttons. I don't know which is better or what it means. But it certainly is jarring to mix and match. A user will have to think "Why is this button different from the other buttons? Oh no, did I do something wrong? Will this mean something else? Why why why???"

ghost · 2021-02-08T20:40:07Z

I agree that it is more practical to make all buttons the same and select them with .button.

Note: PRs that create an entire feature at once are dangerous. You could do the exercise of doing PR in Javascript only, CSS only, JSX / HTML only and so on.
You have to compromise to agree to release half jobs according to your mental plans but it can work in my opinion (The death of design practically, a work of comments and no drawings.).

It is not correct, however, that the CI creates problems that are not related to the files touched, it is like the speech of pure functions, performance problems should not arise in changes to the template (unless there is a direct correlation and I do not I see) 👎

schalkneethling · 2021-02-09T03:36:54Z

The thing I don't like is that the "document footer" has lost its styling. We worked hard to come to the conclusion and design that the document footer would look different and "stand out" more from the last couple of paragraphs of text in the document:

That is a fair point and I was not entirely happy with it. Here is what it looks like now:

On English

On French

schalkneethling · 2021-02-09T03:38:26Z

The other thing I don't like is that switching to English doesn't set the cookie. It's an imperfect thing that changing language with the drop-down actually sets a persistent cookie but it's so very convenient.

Yup, but it was my understanding that that is what we decided to do for now. Perhaps this changed?

schalkneethling · 2021-02-09T03:52:39Z

Another thing I don't understand is; Why is the "Change language" button an outline one?

Addressed.

schalkneethling · 2021-02-09T04:07:48Z

And the tests now all pass 🎉

peterbe · 2021-02-09T19:57:39Z

There's another problem which needs to be addressed. It used to be that the language switcher (which triggers a navigate() from react-router) was at the top of the page so when the <Document> reloads, you would already be at the top of the page. Now, we're going to need to inject some code that scrolls to the top.

peterbe · 2021-02-09T19:58:43Z

I think the unrelated changes that became part of this PR needs to be spun out into its own PRs. If you do that, you won't necessarily need to edit this branch because once you master-merge, the conflicts will automatically resolve.

peterbe · 2021-02-09T20:02:39Z

Why does the "English" choice have to be a hyperlink?
Either it's weird that clicking it doesn't fix remember that you prefer en-US. Or, if clicking it sets a cookie, it's a bit weird that a hyperlink click updates state and it makes you wonder what should happen if you right-click and "Open in a new tab".

If you stumble into a Japanese page, do you really need the English option displayed separately from the drop-down? Only having the drop-down fixes all the problem mentioned in the paragraph above.

peterbe · 2021-02-09T20:05:29Z

Would it be an option to not change how things are working today for Desktop and only on mobile make use the little "language icon" that scrolls to the bottom of the page? On Desktop, where screen real-estate is aplenty, I don't think there's much value in stuffing it to only be accessible at the bottom of the document.

ghost · 2021-02-09T20:08:04Z

Why does the "English" choice have to be a hyperlink?
Either it's weird that clicking it doesn't fix remember that you prefer en-US. Or, if clicking it sets a cookie, it's a bit weird that a hyperlink click updates state and it makes you wonder what should happen if you right-click and "Open in a new tab".

If you stumble into a Japanese page, do you really need the English option displayed separately from the drop-down? Only having the drop-down fixes all the problem mentioned in the paragraph above.

Regarding this I don't know if it is a correct choice but at this point it could turn into a details non form with a list of links inside. In fact, it would have fewer features to manage. I was wondering if there was a CSS-only way to style the links that are the actual URL of the document.

schalkneethling · 2021-02-10T11:35:47Z

Why does the "English" choice have to be a hyperlink?

The idea was for it to be a quick and simple way to switch to English without changing your cookie. @escattone Thoughts?

schalkneethling · 2021-02-10T11:44:20Z

Opened #2875 and #2873

escattone · 2021-02-10T23:03:22Z

@peterbe @schalkneethling

Why does the "English" choice have to be a hyperlink?

The idea was for it to be a quick and simple way to switch to English without changing your cookie. @escattone Thoughts?

Like @schalkneethling said above, I also thought we had agreed within an earlier Zoom meeting that the "switch to English" link would be a convenience for those who'd like quick access to the English page without setting their preferred_locale cookie to English. I favor that approach.

schalkneethling · 2021-02-11T09:49:56Z

There's another problem which needs to be addressed. It used to be that the language switcher (which triggers a navigate() from react-router) was at the top of the page so when the <Document> reloads, you would already be at the top of the page. Now, we're going to need to inject some code that scrolls to the top.

I see that as well. That is unfortunate. Would it be disastrous if we not go via the router and instead do something like window.location.href?

ghost · 2021-02-11T10:42:08Z

instead do something like window.location.href?

This Is probably more mothodic:
https://developer.mozilla.org/en-US/docs/Web/API/Location/assign

I don't understand, but the React router doesn't go away because of the server side rendering? If it is used because of the select I would recommend to do the default submit which however triggers a navigation.
React router might go away as far as I'm concerned (probably reducing some dependency weight metric) ...

schalkneethling · 2021-03-01T12:27:44Z

@peterbe Also showing the language menu on non-English pages. Also, sending a GA event when a user changes language using the language selector. All events are bundled as CATEGORY_LANGUAGE_TOGGLE and this one will say something like:

Changing from the current locale: en-US to /de/docs/Web/HTML/Element/video

peterbe

I don't think the parameters to send to GA are correct. E.g.

eventCategory: "change language", 
eventAction: `Changing from the current locale: ${locale} to ${localeURL}`,
eventLabel: "change-language"

I think it should be something like this instead:

eventCategory: "Language", 
eventAction: "Change preferred language" | "Switch to English" | "Anchor to language choice"
eventLabel: "${locale} to ${translation.locale} on ${currentURL}"

That way, you can aggregate by the category first, and then by the action. And within each action you can then analyze all the specifics.
Look at the examples on https://developers.google.com/analytics/devguides/collection/analyticsjs/sending-hits#using_the_ga_command_queue
And look at the descriptions in the table on https://developers.google.com/analytics/devguides/collection/analyticsjs/events#event_fields

I also think the non-en-US content should have both widgets in the top bar.

"View in English"
(anchor link) "Change language"

I know you mentioned we could let this simmer to build up events stats and then change it but I don't think it's realistic that we remember to do that. Let's get it right the first time if we can.

And the anchor needs to move as demonstrated in the screenshot.

client/src/ui/molecules/language-menu/index.tsx

client/src/ui/molecules/language-toggle/index.tsx

client/src/ui/molecules/language-menu/index.tsx

client/src/ui/molecules/language-toggle/index.tsx

peterbe

I think we should use the URL instead of just the locale.
And in one of the cases, a ga() call never has a chance to execute because the browser will cancel all interactions as it changes the URL.

I collected a bunch of feedback into a "counter-PR" which solves these issues.

client/src/ui/molecules/language-toggle/index.tsx

peterbe · 2021-03-02T19:51:54Z

Feedback as a PR here: https://github.com/schalkneethling/yari/pull/294

feedback on language selector

schalkneethling · 2021-03-03T09:08:41Z

@peterbe Not sure why the tests still fail, I ran ./testing/scripts/functional-test.sh locally and everything passes.

schalkneethling · 2021-03-03T09:16:50Z

Hmmm, now I am also getting Error: Navigation failed because browser has disconnected! locally.

client/src/document/organisms/metadata/index.tsx

peterbe

I carefully tested it in Chrome with the GA debug stuff. And with the SSR built pages. Works swimmingly!

peterbe · 2021-03-03T14:32:50Z

@schalkneethling I see you requested a review from @fiji-flo too so I won't merge it.

peterbe · 2021-03-03T19:47:41Z

testing/tests/headless.test.js

+    await expect(page).toClick("a.view-in-english", {
+      text: "View in English",
+    });
+    await page.waitForNavigation({ waitUntil: "networkidle2" });


This line was the cause of #3116
Sorry. I should have caught that in review. You should only use a line like this when you depend on lazy-loading to come in. At least that's what I found when I worked on the headless tests for site-search.

lex111 · 2021-03-18T15:35:34Z

Sorry for bringing this up, but why did you decide to move the language selector? Previously, it was at the top of the page, it was very convenient and UX-friendly, why move it to the bottom of the page?

peterbe · 2021-03-18T16:43:31Z

Sorry for bringing this up, but why did you decide to move the language selector? Previously, it was at the top of the page, it was very convenient and UX-friendly, why move it to the bottom of the page?

It's still at the top (top right-hand corner area). But now you click it to automatically go to the bottom of the document.

lex111 · 2021-03-18T17:10:49Z

@peterbe I meant why not move language selectbox itself to the top of the document, something like this?

Currently it is extremely inconvenient to switch the language, usually on other websites you can switch language at the top of page.

peterbe · 2021-03-18T18:20:38Z

@peterbe I meant why not move language selectbox itself to the top of the document, something like this?

Currently it is extremely inconvenient to switch the language, usually on other websites you can switch language at the top of page.

Excellent feedback! Thank you for sharing. Much appreciated.

One of the reasons we implemented it the way we implemented it was based on A) that the drop-down widget was clunky on mobile and B) we hoped that it's relatively rare that you'd want to switch. And after all, next to the link that takes you to the bottom of the page, there's a "View in English" link for the translated pages.

@schalkneethling Can you share your thoughts here please?

lex111 · 2021-03-18T18:47:28Z

Actually, you can completely remove the "Change language" button (leaving only eponymous label), and change the language immediately after choosing any other option. For example, something similar is done in the PHP docs, you can check it out yourself, for example, on this page https://www.php.net/manual/en/function.array-chunk.php

* fix: redesign of language selector Implement redesign of language selector and related interactions. This also includes a change to how the underlying grid is implemented. Part of mdn#1486 * fix headless test * change let to const * changes based on review feedback * additional changes based on review * add key the li and fix grid row inconsistency * remove negative tabindex * update to latest implementation * fix failing headless test * fix failing headless test * add missing / at end of test URL * fix overall layout for desktop * updates to overall layout of top of document pages * update grid for standard aka non-document pages * correctly align lang toggle on mobile * handle all 5 possible states of breadcrumb-locale container * instrument view in English and change language links * also show language menu on non English pages * send GA event when changing language via language manu * changes based on review feedback * feedback on language selector * fix failing test * fix failing test * remove react import Co-authored-by: Peter Bengtsson <mail@peterbe.com>

ghost · 2021-12-05T22:24:15Z

@peterbe

One of the reasons we implemented it the way we implemented it was based on A) that the drop-down widget was clunky on mobile and B) we hoped that it's relatively rare that you'd want to switch. And after all, next to the link that takes you to the bottom of the page, there's a "View in English" link for the translated pages.

The scrolling behavior is slow and very disorienting (and potentially bad for people uncomfortable with motions).
(see also #4740 and content/issues/2952)

Users still need to do this A LOT, because docs in their languages aren't complete (or can never be completed), and links in a tranlated page may point to a page in English even if the latter is translated (similar to content/issues/2665). Besides, even if they have translated version, people often switch back and forth to check the original text anyway (check if up-to-date, confusing translations, contributing, etc.).

I would rather it be convenient and comfortable to switch than saving some space.

However, other multilingual docs have already solve this in better ways IMHO:

Wikipedia
- desktop: language list on the left
- mobile: press the "文A" button opens a panel for selecting languages
Blender Manual (and other Sphinx docs)
- desktop: selector is in a fixed place of left panel
- mobile: selector is in hamburger menu (although less visible than on the page)

Actually, you can completely remove the "Change language" button (leaving only eponymous label), and change the language immediately after choosing any other option.

Agreed. This may be a good solution if revert to old design.

fix: redesign of language selector

a328b33

Implement redesign of language selector and related interactions. This also includes a change to how the underlying grid is implemented. Part of #1486

schalkneethling requested review from fiji-flo and peterbe February 7, 2021 14:25

Schalk Neethling added 2 commits February 7, 2021 16:40

fix headless test

576251c

change let to const

1a81269

peterbe suggested changes Feb 8, 2021

View reviewed changes

Schalk Neethling added 2 commits February 9, 2021 05:42

changes based on review feedback

0eb36be

additional changes based on review

9180117

add key the li and fix grid row inconsistency

d42378b

schalkneethling requested a review from peterbe February 9, 2021 04:07

Merge branch 'master' into 1486-move-lang-selector

b62f33b

remove negative tabindex

cbcb12a

send GA event when changing language via language manu

1e834e1

Merge branch 'main' into 1486-move-lang-selector

74deffc

peterbe suggested changes Mar 1, 2021

View reviewed changes

changes based on review feedback

591b187

peterbe reviewed Mar 2, 2021

View reviewed changes

client/src/ui/molecules/language-toggle/index.tsx Outdated Show resolved Hide resolved

peterbe suggested changes Mar 2, 2021

View reviewed changes

client/src/ui/molecules/language-toggle/index.tsx Show resolved Hide resolved

feedback on language selector

99bd309

Schalk Neethling added 3 commits March 3, 2021 10:46

Merge pull request #294 from peterbe/feedback-on-language-selector

3e74a36

feedback on language selector

fix failing test

57f3201

fix failing test

7ba76ee

schalkneethling requested a review from peterbe March 3, 2021 09:08

peterbe reviewed Mar 3, 2021

View reviewed changes

client/src/document/organisms/metadata/index.tsx Outdated Show resolved Hide resolved

peterbe approved these changes Mar 3, 2021

View reviewed changes

remove react import

d801a83

schalkneethling merged commit b965fed into mdn:main Mar 3, 2021

schalkneethling deleted the 1486-move-lang-selector branch March 3, 2021 15:11

peterbe mentioned this pull request Mar 3, 2021

Sporadic errors in headless tests relating to switching language #3116

Closed

peterbe reviewed Mar 3, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: redesign of language selector #2839

fix: redesign of language selector #2839

schalkneethling commented Feb 7, 2021 •

edited

schalkneethling commented Feb 8, 2021

schalkneethling commented Feb 8, 2021 •

edited

peterbe left a comment

peterbe commented Feb 8, 2021

ghost commented Feb 8, 2021 •

edited by ghost

schalkneethling commented Feb 9, 2021

schalkneethling commented Feb 9, 2021

schalkneethling commented Feb 9, 2021

schalkneethling commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

ghost commented Feb 9, 2021 •

edited by ghost

schalkneethling commented Feb 10, 2021

schalkneethling commented Feb 10, 2021

escattone commented Feb 10, 2021

schalkneethling commented Feb 11, 2021 •

edited

ghost commented Feb 11, 2021 •

edited by ghost

schalkneethling commented Mar 1, 2021

peterbe left a comment

peterbe left a comment

peterbe commented Mar 2, 2021

schalkneethling commented Mar 3, 2021

schalkneethling commented Mar 3, 2021

peterbe left a comment

peterbe commented Mar 3, 2021

peterbe Mar 3, 2021

lex111 commented Mar 18, 2021

peterbe commented Mar 18, 2021

lex111 commented Mar 18, 2021

peterbe commented Mar 18, 2021

lex111 commented Mar 18, 2021

ghost commented Dec 5, 2021

fix: redesign of language selector #2839

fix: redesign of language selector #2839

Conversation

schalkneethling commented Feb 7, 2021 • edited

Mobile

Tablet

Desktop

Desktop - no language toggle

Language selector

schalkneethling commented Feb 8, 2021

schalkneethling commented Feb 8, 2021 • edited

peterbe left a comment

Choose a reason for hiding this comment

peterbe commented Feb 8, 2021

ghost commented Feb 8, 2021 • edited by ghost

schalkneethling commented Feb 9, 2021

On English

On French

schalkneethling commented Feb 9, 2021

schalkneethling commented Feb 9, 2021

schalkneethling commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

peterbe commented Feb 9, 2021

ghost commented Feb 9, 2021 • edited by ghost

schalkneethling commented Feb 10, 2021

schalkneethling commented Feb 10, 2021

escattone commented Feb 10, 2021

schalkneethling commented Feb 11, 2021 • edited

ghost commented Feb 11, 2021 • edited by ghost

schalkneethling commented Mar 1, 2021

peterbe left a comment

Choose a reason for hiding this comment

peterbe left a comment

Choose a reason for hiding this comment

peterbe commented Mar 2, 2021

schalkneethling commented Mar 3, 2021

schalkneethling commented Mar 3, 2021

peterbe left a comment

Choose a reason for hiding this comment

peterbe commented Mar 3, 2021

peterbe Mar 3, 2021

Choose a reason for hiding this comment

lex111 commented Mar 18, 2021

peterbe commented Mar 18, 2021

lex111 commented Mar 18, 2021

peterbe commented Mar 18, 2021

lex111 commented Mar 18, 2021

ghost commented Dec 5, 2021

schalkneethling commented Feb 7, 2021 •

edited

schalkneethling commented Feb 8, 2021 •

edited

ghost commented Feb 8, 2021 •

edited by ghost

ghost commented Feb 9, 2021 •

edited by ghost

schalkneethling commented Feb 11, 2021 •

edited

ghost commented Feb 11, 2021 •

edited by ghost