-
Notifications
You must be signed in to change notification settings - Fork 51
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: add a new api-extension to handle localised content from Vendure API #160
feat: add a new api-extension to handle localised content from Vendure API #160
Conversation
Hey hey, sorry for not contact from my side. These days are quite busy. Tomorrow I will take a look at your PR :) |
@@ -0,0 +1,20 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice functionality. @AdamPawlinski will take a look at it from the code part as well.
Could you also update the changelog docs with your change? After this PR will be merged I will add you to the contributors of this project as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @julienTeam it looks really fine for me. It's updating properly and issues with refreshing manually are not connected with this task. I wonder if you try other properties for strategy, without prefix for default language?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @julienTeam it looks really fine for me. It's updating properly and issues with refreshing manually are not connected with this task. I wonder if you try other properties for strategy, without prefix for default language?
Hi Adam, Thank you for your review and comment,
Indeed, I made quite a lot of test changing the strategy property and in the end, it appears that the no_prefix strategy is probably the best choice.
Also, I is good to add a short code in the LocaleSelector.vue file : <a :href="switchLocalePath(lang.code)" @click:="$i18n.setLocaleCookie(lang.code)" > to force the cookie setting to the new language selection without reloading and it works well like that.
It's working well on all pages except when you switch language from the Homepage. In this case, you still need to hard reload the page to force the menu to reload whith the correct categories.
I'm still looking for solutions for this.
Any ideas ?
…update page when switching language
Hi @AdamPawlinski and @Baroshem , I just pushed a hack with the 'no prefix' strategy refixed and a new function in LocaleSelector.vue to force the refresh of the page after language switching, I test on a few browser and it seems to work fine for every situation and all pages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey hey, thanks for the update. I have tested it too and it works! Let's wait however for Adam and Justyna to take a look at this (I would like this feature to be released with the upcoming 1.1.0 release that I plan to release on Friday or Monday :) )
const isLangModalOpen = ref(false); | ||
const availableLocales = computed(() => locales.filter(i => i.code !== locale)); | ||
const languageSwitcher = (lang) => { | ||
console.log(lang) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary console log ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great. Really fine work 👍
@@ -0,0 +1 @@ | |||
[Feature]: implement localised content from Vendure api. [#142] https://github.com/vuestorefront-community/vendure/issues/142 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Baroshem @julienTeam One more thing, shouldn't it be added to a changelog for the 1.0.1 version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clean up the changelogs before releasing so no worries :)
@julienTeam could you please run lint command to lint your files? Apart from that this is ready to be merged :) |
Sorry for that, it should be better now ! |
get Localised content from Vendure shop-API
Description
add a new api-extension file : packages/api-client/src/extensions/localiseConfig.ts :
Also updated the i18_n parameter 'strategy' : 'prefix' to correct the language switching bug
Related Issue
#142
Motivation and Context
Localised content from Vendure e-commerce
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: