Skip to content
This repository has been archived by the owner on Nov 29, 2022. It is now read-only.

Support a new language query param #1031

Merged
merged 3 commits into from
Jan 23, 2018
Merged

Support a new language query param #1031

merged 3 commits into from
Jan 23, 2018

Conversation

zeke
Copy link
Contributor

@zeke zeke commented Jan 22, 2018

This PR allows a ?language= query param to be added to any URL to view that URL in the target language. This does not have an effect on the visitor's language settings (or lack thereof). In other words, if my language is English and I view /docs/api/app?language=fr-FR, I will only see that one page in French. If I click any links or remove the query param from the URL, content will still be displayed in English.

@zeke zeke temporarily deployed to electron-website-pr-1031 January 22, 2018 22:46 Inactive
@zeke
Copy link
Contributor Author

zeke commented Jan 22, 2018

@zeke
Copy link
Contributor Author

zeke commented Jan 22, 2018

This should make it easier for us to diagnose issues during our upcoming call with the Crowdin team

test/index.js Outdated
$('body').text().should.not.include('fenêtres')

$ = await get('/docs/api/browser-window?language=fr-FR')
$('body').text().should.include('fenêtres')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we swap the order of these two tests, so we know the ?language=fr-FR param isn't affecting the language stored in the cookies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

Copy link
Contributor

@vanessayuenn vanessayuenn left a comment

Choose a reason for hiding this comment

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

Small suggestion wrt tests, but feel free to 🚢 as you wish.

@zeke
Copy link
Contributor Author

zeke commented Jan 23, 2018

Oops. The query param is persisting in the cookie. Turns out this is a feature of express-request-language that I hadn't noticed: https://github.com/tinganho/express-request-language/blob/master/README.md#queryname-optional-object

I guess that shows that this testing strategy doesn't account for user state.

Working on a fix.

@zeke zeke added enhancement New feature or request area:i18n Issues or Pull Requests related to i18n labels Jan 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:i18n Issues or Pull Requests related to i18n enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants