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

Language switching and local storage persistence #735

Merged
merged 3 commits into from
Nov 26, 2017

Conversation

ajparise
Copy link
Member

Supplements #446, #730.

  • Adds language switching to footer
    image
  • Language list is generated dynamically from TAPi18n.getLanguages()
  • Persisted via LocalStorage
  • Removes query param for forcing language

image
image

@ajparise ajparise self-assigned this Nov 22, 2017
@lpatmo lpatmo added the [state] in-review the implementation that addresses this issue is up for review label Nov 22, 2017
@nalbina
Copy link
Contributor

nalbina commented Nov 22, 2017

Hey @ajparise, thanks for your PR. Does it need a review yet?

@ajparise
Copy link
Member Author

@nalbina It's ready for a review. Thanks!

Copy link
Contributor

@nalbina nalbina left a comment

Choose a reason for hiding this comment

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

Looks good so far. I added some comments to naming and behaviour when clicking on an already selected language.

const languageCode = event.target.dataset.langCode
const languageName = event.target.dataset.langName

TAPi18n.setLanguage(languageCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like setting the same language again should be a noop. right now I am getting a notification for each click.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense and was intentional, but valid point. I added a guard clause.

console.log(error);
})
.always(() => {
localStorage.setItem('lang', TAPi18n.getLanguage());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call the localstorage item also languageCode to stay in sync with the rest? :)

Copy link
Member Author

@ajparise ajparise Nov 22, 2017

Choose a reason for hiding this comment

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

Good catch, updated in all related files.

@@ -9,14 +9,14 @@ if (Meteor.isClient) {
}

const defaultLang = 'en'
const urlLang = FlowRouter.getQueryParam('lang')
const localStorageLang = localStorage.getItem('lang');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call the localstorage item also languageCode to stay in sync with the rest? :)

Copy link
Member

@distalx distalx left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@distalx
Copy link
Member

distalx commented Nov 26, 2017

Great work @ajparise 👏
thanks for the review @nalbina 🥇

@distalx distalx merged commit 95f6f1a into codebuddies:staging Nov 26, 2017
@lpatmo lpatmo added [state] closed the issue is now closed, see comments for more information and removed [state] in-review the implementation that addresses this issue is up for review labels Nov 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[state] closed the issue is now closed, see comments for more information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants