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

[FEATURE] MultiSite Context should include locale object instead of id #712

Closed
nx-renezwinge opened this issue Sep 6, 2022 · 3 comments
Closed

Comments

@nx-renezwinge
Copy link

Is your feature request related to a problem? Please describe.
With version 2.2.0 we have now the MultiSite context, which includes the current site object, locale id and a buildUrl method.
This concept is much better then before and makes it easier to create full URLs, but i get with it only URLs with the locale id and not with locale alias if this is configured. The method has the localeRef parameter to have the possibility to set a custom locale path, but this means for every usage of buildUrl we must get the alias from the config and set it over the parameter.

Describe the solution you'd like
Instead only saving the locale id into the MultiSite context, save the complete locale object in the context, then change in the useMultiSite hook the following line: localeRef ? localeRef : locale?.alias || locale?.id
I don't see a reason, why this is not implemented, because old buildPathWithUrlConfig method was checking for configured alias and this method was removed.

Describe alternatives you've considered
Alternative is to use the localeRef parameter, which is not so useful in the complete project. We refactored the context, hook and tests to save the locale object in the MultiSite context to make the buildUrl more usable.

@bendvc
Copy link
Collaborator

bendvc commented Sep 6, 2022

Hi @nx-renezwinge,

Thanks for the input. Your suggestion makes sense. I'd like to run it by the eng that did this recent work. He'll be back in the office Sept 12th @adamraya.

One thing that I would like to mention, is that we do have a future work item to move this feature into the SDK or some other reusable library so that updates and be more easily consumed. So that is probably he time we'd be looking at taking in your suggestion.

I'll keep this issue open so we can get a response from Adam.

Thanks!

Ben

Edit: Updated date my colleague will be back in the office.

@adamraya
Copy link
Collaborator

adamraya commented Sep 14, 2022

Thank your @nx-renezwinge for reporting the issue. You are correct. PR #667 introduced the new MutliSite context but did not consider the locale alias configuration.

The suggested solution has been implemented in #716 .
Now the <MutiSiteContext> has the complete locale object available. If the locale.alias is defined in the sites.js configuration file, using the locale.alias in the URL is prioritized over the locale.id.

The solution is merged and will be included in the next release.

New generated projects from the future release will include the fix, but existing projects will need to add the fix manually.
It looks like you already applied the refactor in your project, but let me know if I can help to apply the fix.

@nx-renezwinge
Copy link
Author

Hi @adamraya,
thx for the fast fix :) The changes looking mostly the same.
On the next update we add your code then :)

Best regards René

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

No branches or pull requests

3 participants