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

[OPTIONAL] - Newssite - updates to support different languages / html direction in the future #248

Merged
merged 8 commits into from
Jun 29, 2023

Conversation

flashdesignory
Copy link
Contributor

rebuild to support en, jp & ar languages. This re-uses one set of source files.

@rniwa - selecting more than one of these, causes the security error in safari to appear again (history.replaceState).

But it's here for opinions...

Chrome:
before:
before_chrome
after:
after_chrome

Safari
before:
before_safari
after:
after_safari

Firefox:
before:
before_firefox
after:
after_firefox

Standalone previews:
English: https://flashdesignory.github.io/news-site-next-static/index.html
Japanese: https://flashdesignory.github.io/news-site-next-static/index.html?lang=jp
Arabic: https://flashdesignory.github.io/news-site-next-static/index.html?lang=ar&dir=rtl

@kara

@flashdesignory flashdesignory mentioned this pull request Jun 20, 2023
@rniwa
Copy link
Member

rniwa commented Jun 20, 2023

At least Japanese version has a lot of weird translations. Also, what are before/after numbers? Are the numbers for en content?

@flashdesignory
Copy link
Contributor Author

At least Japanese version has a lot of weird translations. Also, what are before/after numbers? Are the numbers for en content?

Weird translations was my goal 😄 (translated some English dummy text and lorem ipsum into Japanese).

The before / after numbers are comparing current build in main (before) with this update (after), using english translation to ensure the same language is used.

@flashdesignory flashdesignory changed the title [OPTIONAL] - Newssite with en, jp & ar [OPTIONAL] - Newssite with en, jp & ar (LTR & RTL) Jun 22, 2023
@flashdesignory
Copy link
Contributor Author

this pr could also be used if we just want a RTL workload (independent from languages discussions)

@bgrins
Copy link
Contributor

bgrins commented Jun 28, 2023

We discussed landing a first version with only the code refactoring and then handling the actual translations / workload changes separately

@flashdesignory
Copy link
Contributor Author

We discussed landing a first version with only the code refactoring and then handling the actual translations / workload changes separately

Thanks @bgrins - translations have been removed.

@flashdesignory flashdesignory changed the title [OPTIONAL] - Newssite with en, jp & ar (LTR & RTL) [OPTIONAL] - Newssite - updates to support different languages / html direction in the future Jun 28, 2023
Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

This looks good to me!
I suggested another structure for the data provider, tell me what you think of that. But otherwise this seems solid.

const defaultLanguage = "en";
const urlParams = new URLSearchParams(window.location.search);
const lang = urlParams.get("lang") ?? "en";
const dir = urlParams.get("dir") ?? "ltr";
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be work for later, but in our project we're infering the direction from the language directly: https://github.com/firefox-devtools/profiler/blob/8ab31e7e35f0075ea93ef76784697f84185de9f1/src/app-logic/l10n.js#L53-L57
Might be worth doing the same thing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's do that one later....

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we can do it once we add an actual RTL language.

const dir = urlParams.get("dir") ?? "ltr";

document.documentElement.setAttribute("dir", dir);
document.documentElement.setAttribute("lang", defaultLanguage);
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
document.documentElement.setAttribute("lang", defaultLanguage);
document.documentElement.setAttribute("lang", lang);

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

export const DataContextProvider = ({ children }) => {
const defaultLanguage = "en";
const urlParams = new URLSearchParams(window.location.search);
const lang = urlParams.get("lang") ?? "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 lang = urlParams.get("lang") ?? "en";
const lang = urlParams.get("lang") ?? defaultLanguage;

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

Comment on lines 8 to 12
const contentImport = { en: contentEn };
const settingsImport = { en: settingsEn };
const footerImport = { en: footerEn };
const buttonsImport = { en: buttonsEn };
const linksImport = { en: linksEn };
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd rather do a structure such as:

const strings = {
  en: {
    content: contentEn,
    settings: settingsEn,
    footer: footerEn,
    buttons: buttonsEn,
    links: linksEn,
  },
};

Because it makes it straightforward to check for the existence of the locale later, that is instead of:

const lang = urlParams.get("lang") ?? defaultLanguage;

we'd have

const langFromUrl = urlParams.get("lang");
const lang = langFromUrl && langFromUrl in strings ? langFromUrl : defaultLanguage;

and then get rid of the condition in the return value:

const value = {
  lang,
  dir,
  ...strings[lang],
};

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like it - changed.

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

Comment on lines 12 to 13
const data = useDataContext();
const { settings } = data;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
const data = useDataContext();
const { settings } = data;
const { settings } = useDataContext();

Copy link
Contributor

Choose a reason for hiding this comment

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

(ditto below)

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

Copy link
Contributor

@julienw julienw left a comment

Choose a reason for hiding this comment

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

thanks for the changes!

@flashdesignory flashdesignory merged commit 9025eb1 into WebKit:main Jun 29, 2023
4 checks passed
@flashdesignory flashdesignory deleted the newssite-lang branch June 29, 2023 14:11
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.

4 participants