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

fix: locale detection for icons #81

Merged
merged 3 commits into from
Jan 11, 2024
Merged

fix: locale detection for icons #81

merged 3 commits into from
Jan 11, 2024

Conversation

imprashast
Copy link
Contributor

Change the way we detect locale based on the current NMP RFC

@imprashast imprashast requested review from digitalsadhu and a team January 9, 2024 14:13
Copy link

@digitalsadhu digitalsadhu 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, suggested a couple changes

src/utils/i18n.ts Outdated Show resolved Hide resolved
return getSupportedLocale(htmlLocale);
const htmlLocale = document?.documentElement?.lang;
const hostLocale = detectByHost();
return getSupportedLocale(htmlLocale ?? hostLocale);

Choose a reason for hiding this comment

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

For this fallback approach to work in most cases, you would need to validate/parse document.documentElement.lang since in all cases I've seen, the value is set, just not always correctly. no is a typical invalid value. Ideally we'd use a library for this but thats extra kb we could do without.
You could add a detectByLangAttribute() function that checks for valid en, fi, nb, sv and da values perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I have added a fallback which if the html lang is incorrect, fallback to detection using hostname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Console warning when the detection fails:
Screenshot 2024-01-10 at 10 50 36

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like this?

Unsupported "${htmlLocale}" locale set in lang attribute on the html tag.
Falling back to locale based on hostname.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Printing htmlLocale isn't really useful since someone checking this on console can easily see what the html lang is set to. Wordings I'll tweak a bit 👍

@imprashast imprashast merged commit f97d09f into next Jan 11, 2024
1 check passed
@imprashast imprashast deleted the fix_i18n branch January 11, 2024 07:41
github-actions bot pushed a commit that referenced this pull request Jan 11, 2024
## [1.4.1-next.1](v1.4.0...v1.4.1-next.1) (2024-01-11)

### Bug Fixes

* locale detection for icons ([#81](#81)) ([f97d09f](f97d09f))
github-actions bot pushed a commit that referenced this pull request Feb 2, 2024
# [1.5.0](v1.4.0...v1.5.0) (2024-02-02)

### Bug Fixes

* locale detection for icons ([#81](#81)) ([f97d09f](f97d09f))
* update oikotie market icons ([#83](#83)) ([d4bad16](d4bad16))
* update strings for logout, picture-stack and organize ([#90](#90)) ([f381be3](f381be3))

### Features

* add 16px icons and update all svgs ([#91](#91)) ([a5f5794](a5f5794))
* add missing 16px icons and a geometric-shapes icon ([#84](#84)) ([c22fd1b](c22fd1b))
* add missing icons ([#88](#88)) ([f5db3ac](f5db3ac))
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

3 participants