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(presenter): make i18n locale extractable from url #335

Merged

Conversation

Elweyn
Copy link
Member

@Elweyn Elweyn commented Mar 14, 2024

🍰 Pullrequest

Issues

  • None

Todo

  • None

@Elweyn Elweyn linked an issue Mar 14, 2024 that may be closed by this pull request
@Elweyn Elweyn self-assigned this Mar 16, 2024
@Elweyn Elweyn changed the title [WIP] feat(presenter): make i18n for page titles [WIP] feat(presenter): make i18n locale storable Mar 20, 2024
@Elweyn Elweyn changed the title [WIP] feat(presenter): make i18n locale storable [WIP] feat(presenter): make i18n locale extractable from url Mar 22, 2024
@Elweyn Elweyn changed the title [WIP] feat(presenter): make i18n locale extractable from url feat(presenter): make i18n locale extractable from url Mar 22, 2024
Copy link
Member

@ulfgebhardt ulfgebhardt 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 very good. Please check if you can unify the language defintions so that it wold be easier to add new languages.
If this is a big overhead which prevents merging, we can merge it and have a new task for that.

Copy link
Member

@ulfgebhardt ulfgebhardt left a comment

Choose a reason for hiding this comment

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

I can only use http://localhost:3000/en/impressum but not http://localhost:3000/de/impressum which I expected to be able.
Since de is the default language this should not imply a change for the page, still I think every valid language should be acceptable as the language parameter.

In general this is the right approach it just needs to be finished properly to be production ready.
There is a problem resulting from the manual language switch, which might be subject of another PR:
When you are on http://localhost:3000/en/impressum and select de in the menu the URL does not change, which then sets the content unaligned with the given url - so assume the following user flow:

  • User goes to page
  • Wants to send link to friend
  • Notices - oh friend only speaks german
  • Switches language
  • Sends link

-> This scenario falls short here.

presenter/renderer/app.ts Outdated Show resolved Hide resolved
presenter/src/components/language/LanguageSelector.vue Outdated Show resolved Hide resolved
presenter/src/components/language/LanguageSelector.vue Outdated Show resolved Hide resolved
presenter/locales/extractLocale.ts Outdated Show resolved Hide resolved
presenter/renderer/+onBeforeRoute.ts Outdated Show resolved Hide resolved
presenter/renderer/+onBeforeRoute.ts Outdated Show resolved Hide resolved
Copy link
Member

@ulfgebhardt ulfgebhardt left a comment

Choose a reason for hiding this comment

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

A further test revealed that page navigation loses the locale.

If you are on http://localhost:3000/en/impressum and navigate to http://localhost:3000/ locale is de again. This is actually an regression from previous functionality where the locale set in the browser was preserved.

@Elweyn Elweyn merged commit d89cb76 into master Mar 26, 2024
41 checks passed
@Elweyn Elweyn deleted the 208-feature-implement-page-title-translation-and-vike-language branch March 26, 2024 12:40
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.

2 participants