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(i18n): initial setup #711

Merged
merged 2 commits into from
Apr 5, 2022
Merged

Conversation

pavanjoshi914
Copy link
Contributor

@pavanjoshi914 pavanjoshi914 commented Mar 19, 2022

Solves #600

Type of change

New feature (non-breaking change which adds functionality)

Describe the changes you have made in this PR -

setup i18next react-i18next browser-language-detector
create locale switcher
styling for both light and dark mode
translate heading
enable JSON imports in config
language persistence throughout the component
language persistence between different sessions
browser language detections
format JSON translation file using built-in eslint JSON formatted and run automated using lintstaged

Screenshots of the changes (If any) -

image
image
image
image

Updates

locale switcher shifted under settings
image
locale switcher on Intro shown only under development mode
store locale in extension's storage as well

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Checklist:

  • My code follows the style guidelines of this project and performed a self-review of my own code
  • New and existing tests pass locally with my changes

@pavanjoshi914
Copy link
Contributor Author

@bumi @escapedcat initial setup for i18n

@pavanjoshi914
Copy link
Contributor Author

if you have gone through the documentation! either we can use nesting of keys or react i18next namespaces

currently, I am using namespace. if we wish to use nesting of keys as a key management strategy we can do that as well

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

Really cool, thanks! Some minor comments.
@bumi do we want use Weblate so others can help with further translations?

package.json Outdated Show resolved Hide resolved
src/app/components/LocaleSwitcher/LocaleSwitcher.tsx Outdated Show resolved Hide resolved
src/i18n/locales/en.json Show resolved Hide resolved
src/app/components/Navbar/Navbar.tsx Outdated Show resolved Hide resolved
},
"lint-staged": {
"src/**/*.{js,jsx,ts,tsx}": [
"src/**/*.{js,jsx,ts,tsx,json}": [
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

Copy link
Contributor

@escapedcat escapedcat left a comment

Choose a reason for hiding this comment

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

Awesome job @pavanjoshi914 !

src/app/components/LocaleSwitcher/LocaleSwitcher.tsx Outdated Show resolved Hide resolved
@pavanjoshi914
Copy link
Contributor Author

Squashed all the commits into a single commit. @bumi PR is ready to be merged!

@pavanjoshi914
Copy link
Contributor Author

Really cool, thanks! Some minor comments. @bumi do we want to use Weblate so others can help with further translations?

regarding weblate! currently, we can just focus on completing i18n with a single alternate language. setup for continuous localization can be done once we are finished with this
https://gist.github.com/pavanjoshi914/a65af473209541c3a5cfecbac0f6e70f#continuous-localization

@escapedcat
Copy link
Contributor

Applied for the "Libre hosting" plan on weblate already

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

I think we should save the selected language in the settings storage. Otherwise it will get lost after the app reloads.

You can see how to save the settings there:

async function saveSetting(

@pavanjoshi914
Copy link
Contributor Author

pavanjoshi914 commented Mar 23, 2022

I think we should save the selected language in the settings storage. Otherwise it will get lost after the app reloads.

You can see how to save the settings there:

async function saveSetting(

sure so we will have incoming locale from three places

  1. browser
  2. language switcher
  3. settings

Case 1. storing browser detected language or fallback language

when a user installs the app we let the browser detector, detect the language and use that language. either browser detected language will be used and if not available fallback language :en will be used

resolvedLanguage can handle this case, we exported the final resolved language under default settings

export const DEFAULT_SETTINGS = {
  websiteEnhancements: true,
  userName: "",
  locale: i18n.resolvedLanguage,
};

Case 2: store language changed via locale switcher

  1. locale switcher under settings
    passing saveSetting function as props and executed in LocaleSwitcher.tsx
<LocaleSwitcher saveSettings={saveSetting} />
 if (dropdownLang !== newLanguage) {
      setDropdownLang(newLanguage);
      i18n.changeLanguage(newLanguage);
      props.saveSettings({ locale: newLanguage });
    }
  1. locale switcher under intro section
    I can't pass a function as props for this locale switcher. one idea I think is to refactor settings to make it exportable and make the function accessible.

    I am still learning React :) what can be the best possible approach for handling case 2 @bumi @escapedcat any ideas?

@bumi
Copy link
Collaborator

bumi commented Mar 23, 2022

Can you elaborate on 2.? Why doesn't it work the same way as in the settings on the welcome page?

@pavanjoshi914
Copy link
Contributor Author

Can you elaborate on 2.? Why doesn't it work the same way as in the settings on the welcome page?

I made a silly mistake! I thought saveSetting is the function which is providing save functionality and I was trying this to pass as props in localeSwitcher component. it was just a wrapper for const response = await api.setSetting(setting);

async function saveSetting(
    setting: Record<string, string | number | boolean>
  ) {
    const response = await api.setSetting(setting);
    setSettings(response);
  }

did required changes @bumi @escapedcat kindly review

@escapedcat escapedcat requested a review from bumi March 24, 2022 08:19
@escapedcat
Copy link
Contributor

I made a silly mistake

image

Copy link
Collaborator

@bumi bumi left a comment

Choose a reason for hiding this comment

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

From the code I don't fully understand where we load the language from the settings.
Can you point me to that?

Also how do we roll this out? Should we only show the language switcher in dev mode until we have a decent amount of the app translated?

src/app/router/Welcome/Welcome.tsx Show resolved Hide resolved
@pavanjoshi914
Copy link
Contributor Author

if you have gone through the documentation! either we can use nesting of keys or react i18next namespaces

currently, I am using namespace. if we wish to use nesting of keys as a key management strategy we can do that as well

...

@bumi
Copy link
Collaborator

bumi commented Mar 24, 2022

sorry, my bad. forgot...sure. really good, let's add that to the wiki? and add a link in the i18config in code to that wiki page?

@bumi bumi closed this Mar 24, 2022
@bumi bumi reopened this Mar 24, 2022
@bumi
Copy link
Collaborator

bumi commented Mar 24, 2022

(ups and I don't even hit the right button this morning, sorry. :) )

@pavanjoshi914
Copy link
Contributor Author

sorry, my bad. forgot...sure. really good, let's add that to the wiki? and add a link in the i18config in code to that wiki page?

sure , but before that i have to do few revisions to make it more formal.

@escapedcat
Copy link
Contributor

Also how do we roll this out? Should we only show the language switcher in dev mode until we have a decent amount of the app translated?

Good point. We can also add alpha to the settings-label, already show it and open an issue to move all texts step by step to translation-strings.

@pavanjoshi914
Copy link
Contributor Author

pavanjoshi914 commented Mar 24, 2022

From the code I don't fully understand where we load the language from the settings. Can you point me to that?

Also how do we roll this out? Should we only show the language switcher in dev mode until we have a decent amount of the app translated?

I made a silly mistake

image

haha nice one @escapedcat 😁

@AaronDewes
Copy link
Contributor

@bumi If you're looking for a translation platform, I recommend hosted.weblate.org, they are free for open source projects if you apply. I'm using it for Citadel and it works great

@pavanjoshi914
Copy link
Contributor Author

pavanjoshi914 commented Mar 24, 2022

I think we should save the selected language in the settings storage. Otherwise it will get lost after the app reloads.

I think this will not happen! what does it mean by reloading? (refreshing browser?).
what the browser detector plugin does is, stores the language by default in local storage, that is why it will still be persisted.

And uses the following sequence in priority to resolve the locale.

cookie
localStorage
navigator
querystring (append ?lng=LANGUAGE to URL)
htmlTag
path
subdomain

image

@dylancom
Copy link
Contributor

@pavanjoshi914 Going to test it now!

@dylancom
Copy link
Contributor

dylancom commented Mar 29, 2022

The vertical alignment of the LanguageSwitcher seems off. It also doesn't show the pointer cursor while hovering over it.
We already have a component by the way that you can use from components/form

{process.env.NODE_ENV === "development" && <DevMenu />}

{process.env.NODE_ENV === "development" && (
<Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Troughout the project we use the short syntax for fragments that doesn't require an import.
https://reactjs.org/docs/fragments.html#short-syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import { useState } from "react";
import type { ChangeEvent } from "react";
import i18n from "i18next";
import "../../../i18n/i18nConfig";
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace the two i18n imports by:
import i18n from "../../../i18n/i18nConfig";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

import api from "../../../common/lib/api";

export default function LocaleSwitcher() {
const [dropdownLang, setDropdownLang] = useState(i18n.language || "en");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const [dropdownLang, setDropdownLang] = useState(i18n.language || "en");
const [dropdownLang, setDropdownLang] = useState(i18n.language || i18n.options.fallbackLng[0]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/app/components/LocaleSwitcher/LocaleSwitcher.tsx Outdated Show resolved Hide resolved
@pavanjoshi914
Copy link
Contributor Author

The vertical alignment of the LanguageSwitcher seems off. It also doesn't show the pointer cursor while hovering over it. We already have a component by the way that you can use from components/form

not sure what I have to do here! any screenshot which can help to understand where is the problem?

@pavanjoshi914
Copy link
Contributor Author

@dylancom

@escapedcat
Copy link
Contributor

The vertical alignment of the LanguageSwitcher seems off. It also doesn't show the pointer cursor while hovering over it. We already have a component by the way that you can use from components/form

not sure what I have to do here! any screenshot which can help to understand where is the problem?

I think he meant you can use this component instead of a normal select:
https://github.com/getAlby/lightning-browser-extension/blob/test/select-account-after-account-delete/src/app/components/form/Select/index.tsx

You can find an example how to use it here:

@pavanjoshi914
Copy link
Contributor Author

The vertical alignment of the LanguageSwitcher seems off. It also doesn't show the pointer cursor while hovering over it. We already have a component by the way that you can use from components/form

not sure what I have to do here! any screenshot which can help to understand where is the problem?

I think he meant you can use this component instead of a normal select: https://github.com/getAlby/lightning-browser-extension/blob/test/select-account-after-account-delete/src/app/components/form/Select/index.tsx

You can find an example how to use it here:

already using! but i refactored by default class present with component!

@pavanjoshi914
Copy link
Contributor Author

already using! but i refactored by default class present with component!

the only important class was h-14 in that default class (w-full h-14 border-gray-300 rounded-md shadow-sm text-gray-700 bg-white), added that too! not sure if this is what Dylan willing to have

@escapedcat
Copy link
Contributor

already using! but i refactored by default class present with component!

👍 Ah sorry, didn't see.

Might be good having default styles on a select which can be extended or overwritten, but we can take care of that later on.

@pavanjoshi914
Copy link
Contributor Author

already using! but i refactored by default class present with component!

👍 Ah sorry, didn't see.

Might be good having default styles on a select which can be extended or overwritten, but we can take care of that later on.

yup but the class has w-full which we don't want in case of language switcher also tried overwriting it! nothing happened

@dylancom
Copy link
Contributor

dylancom commented Apr 1, 2022

@pavanjoshi914 What is the problem with the w-full class? You let the width of the component to be decided by the parent that it's wrapped in. It might need some updated styling if it's too high now, as it has been styled a while ago and not used in prod yet. But I would keep default styles instead of passing in custom classes, and let a parent element decide how it's aligned, what it's width is etc.

@pavanjoshi914
Copy link
Contributor Author

w-full covers entire screen
image

@pavanjoshi914
Copy link
Contributor Author

@pavanjoshi914 What is the problem with the w-full class? You let the width of the component to be decided by the parent that it's wrapped in. It might need some updated styling if it's too high now, as it has been styled a while ago and not used in prod yet. But I would keep default styles instead of passing in custom classes, and let a parent element decide how it's aligned, what it's width is etc.

so I can wrap the select element in div tag to overwrite few of the styles.

@dylancom
Copy link
Contributor

dylancom commented Apr 1, 2022

w-full means width: 100%; which means take 100% width of the parent container. So if your parent container is 400px wide, w-full means a width of 400px.

setup i18next react-i18next browser-language-detector
create locale switcher
styling for both light and dark mode
translate heading
enable json imports in tsconfig
language persistence throughout different components
language persistence between different sessions
browser language detections
format JSON translation file using built-in eslint JSON formatted and run automated using lintstaged
format existing json file
place language switcher on intro and under settings (for intro under development mode)
store locale in extensions storage
signed-off-by: pavan joshi <pavanj914@gmail.com>
@pavanjoshi914
Copy link
Contributor Author

@dylancom can you review it again!

@dylancom
Copy link
Contributor

dylancom commented Apr 1, 2022

If your screen width is smaller than 1300px in the Welcome screen, then you can't click the language switcher. (elements overlapping)

signed-off-by: pavan joshi <pavanj914@gmail.com>
@pavanjoshi914
Copy link
Contributor Author

If your screen width is smaller than 1300px in the Welcome screen, then you can't click the language switcher. (elements overlapping)

oops! corrected

@pavanjoshi914
Copy link
Contributor Author

@bumi kindly review and merge

@bumi bumi merged commit 369c507 into getAlby:master Apr 5, 2022
@pavanjoshi914 pavanjoshi914 deleted the i18n-inital-setup branch April 5, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants