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

Use documentLocaleSettings #156

Merged
merged 4 commits into from
Sep 1, 2023
Merged

Conversation

bearfriend
Copy link
Contributor

@bearfriend bearfriend commented Aug 31, 2023

Lots of things are listening for changes to the common DocumentLocaleSettings instance, so use that instead of local currentLang, which can get out of sync of the lang is changed before the next reset.

d2l-localize-behavior blew up in all kinds of weird ways when I switched to the new fixture without this.

@bearfriend bearfriend requested a review from a team as a code owner August 31, 2023 23:05
@bearfriend bearfriend force-pushed the dgleckler/documentLocaleSettings branch from 524b84c to 81b18dc Compare August 31, 2023 23:08
@bearfriend bearfriend force-pushed the dgleckler/documentLocaleSettings branch from 56fd48c to 9ecc8f2 Compare September 1, 2023 00:27
Comment on lines +63 to +65
opts.lang ??= '';
if (documentLocaleSettings.lamguage !== opts.lang) {
document.documentElement.lang = opts.lang;
Copy link
Contributor Author

@bearfriend bearfriend Sep 1, 2023

Choose a reason for hiding this comment

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

  1. Avoids coercion of nullish values to strings (i.e. <html lang="null" ...>)
  2. Compares against documentLocaleSettings for guaranteed latest change
  3. Sets document.documentElement.lang to guarantee all values are in sync (documentLocaleSettings is observing this value)

Copy link
Member

@dlockhart dlockhart left a comment

Choose a reason for hiding this comment

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

Cool, good call.

@bearfriend bearfriend merged commit 0bdfd5d into main Sep 1, 2023
2 checks passed
@bearfriend bearfriend deleted the dgleckler/documentLocaleSettings branch September 1, 2023 13:19
@ghost
Copy link

ghost commented Sep 1, 2023

🎉 This PR is included in version 0.31.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ghost ghost added the released label Sep 1, 2023
@@ -36,8 +36,9 @@
"access": "public"
},
"dependencies": {
"@rollup/plugin-node-resolve": "^15",
"@brightspace-ui/intl": "^3",
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we can't use @brightspace-ui/testing in intl, or would that still work? 🤔 Looks like we use mocha directly anyways so maybe doesn't matter

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think that's what it implies. I'm OK with that, since yeah keeping intl bare bones seems like a good call anyway given how low-level it is.

document.documentElement.setAttribute('lang', opts.lang);
currentLang = opts.lang;
opts.lang ??= '';
if (documentLocaleSettings.lamguage !== opts.lang) {
Copy link
Contributor

@svanherk svanherk Sep 5, 2023

Choose a reason for hiding this comment

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

I assume this should be "language" and this is working right now because this is always true?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants