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

feat(lng): Added change language on the fly #456

Merged
merged 11 commits into from Apr 23, 2020

Conversation

priscilawebdev
Copy link
Contributor

@priscilawebdev priscilawebdev commented Apr 5, 2020

Change Language on the fly

image

image

@priscilawebdev priscilawebdev changed the title WIP feat(lng): Added change language on the fly feat(lng): Added change language on the fly Apr 8, 2020
@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #456 into master will decrease coverage by 0.80%.
The diff coverage is 69.64%.

@@            Coverage Diff             @@
##           master     #456      +/-   ##
==========================================
- Coverage   84.22%   83.41%   -0.81%     
==========================================
  Files         141      142       +1     
  Lines        1014     1067      +53     
  Branches      210      211       +1     
==========================================
+ Hits          854      890      +36     
- Misses        127      145      +18     
+ Partials       33       32       -1     
Impacted Files Coverage Δ
src/App/load-dayjs-locale.ts 25.92% <ø> (ø)
src/components/Header/HeaderRight.tsx 79.16% <ø> (ø)
src/components/Icon/Icon.tsx 100.00% <ø> (ø)
src/design-tokens/ThemeContext.ts 100.00% <ø> (ø)
src/components/LanguageSwitch/LanguageSwitch.tsx 63.04% <63.04%> (ø)
i18n/config.ts 100.00% <100.00%> (ø)
src/design-tokens/ThemeProvider.tsx 100.00% <100.00%> (ø)
src/muiComponents/MenuItem/MenuItem.tsx 100.00% <100.00%> (ø)
src/utils/package.ts 96.00% <100.00%> (ø)
src/components/Icon/styles.ts 88.88% <0.00%> (ø)
... and 1 more

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Looks great ! But few things I think worth to highlight

  • We don't persist the language, on refresh I lose my previous selection, as we do with dark mode.
  • Mobile view does not look fine, the menu + dark mode does not fit.

What do you think?

@juanpicado
Copy link
Member

I just merged French, I think we need also to update it here.

@priscilawebdev priscilawebdev force-pushed the feat/change-language-on-the-fly branch from 119d918 to d52a2a5 Compare April 19, 2020 18:36
@priscilawebdev
Copy link
Contributor Author

priscilawebdev commented Apr 19, 2020

@juanpicado thanks for the nice feedback! I just added the current language to localStorage, so it should be fine now if you reload the page ...

I also updated the menu background color in dark mode 😉

Regarding the header on mobile devices, I plan to refactor it by adding a mobile menu, because atm it is impossible to add another icon / information ... as you said, it looks odd ... I will do this in a follow-up PR ... but in the meantime, the component switchLanguage will be hidden on smaller devices

I'm not having much time lately, but I will try to do that as soon as I have some free time 🙂

@sonarcloud
Copy link

sonarcloud bot commented Apr 19, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@verdacciobot
Copy link

Thanks for your PR, the @verdaccio/ui package will be accessible from here for testing purposes:

npm install @verdaccio/ui-theme@v1.5.1-67bd520-pr456.0 --registry https://registry.verdaccio.org

@juanpicado
Copy link
Member

@juanpicado thanks for the nice feedback! I just added the current language to localStorage, so it should be fine now if you reload the page ...

I also updated the menu background color in dark mode 😉

Regarding the header on mobile devices, I plan to refactor it by adding a mobile menu, because atm it is impossible to add another icon / information ... as you said, it looks odd ... I will do this in a follow-up PR ... but in the meantime, the component switchLanguage will be hidden on smaller devices

I'm not having much time lately, but I will try to do that as soon as I have some free time 🙂

Perfect, good solution 👍

Copy link
Member

@juanpicado juanpicado left a comment

Choose a reason for hiding this comment

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

Awesome 💯

@juanpicado juanpicado merged commit 675ee98 into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat/change-language-on-the-fly branch April 23, 2020 06:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants