-
Notifications
You must be signed in to change notification settings - Fork 15
MWPW-181321 mas-locale-picker #481
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
base: main
Are you sure you want to change the base?
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
Commits
|
|
|
||
| handleLocaleChange(locale) { | ||
| this.locale = locale; | ||
| this.dispatchEvent( |
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.
updating a Store value would be an option here.
handling events is more verbose.
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 didn't want store logic to be within the component, typically the same component is used for different purposes, with different stores update
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.
You are right, I thought about it after I commented 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.
i thought about your comment though :-p and i could pass in an "abstract" store to the component that the later blindly updates. It would save some lines of code, but the event based setup is kind of clear to read imo
|
note a separate ticket has been tracked for flag icons (for now emoticons) https://jira.corp.adobe.com/browse/MWPW-185464 |
|
will try to fix nala tests |
honstar
left a comment
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.
Found one issue, the rest LGTM.
no, locale in the url is bound to top right language section, so when you change region (placeholder or fragment variation), this should stay the same
this should be fixed (alphabetical sort of language/region labels)
only through the variation table of a given fragment
this should be fixed |
|
@npeltier without region, the prices on regional variations are in default language/country. For english for example, every country (Argentina, Egypt, etc) has prices in US dollars instead of local currency when you edit them Also, after creating this AU variation, i go back to Fragments table. It lands me back to default language table (meaning or suggesting to the user that they are back to en_US), but if i click placeholders, it is landing me on en_AU (with selected region). This is misleading experience, not to know in every moment where i am exactly and what am i looking at Then being in AU region, i clicked to change the region to US since i wanted to be there in the first place, i got "could not load the placeholders" toast and stayed on AU one
|
|
@afmicka fixed currency & region state cleanup |
|
@npeltier prices are still in US$ in the editor for existing variations. Only preview is fixed |
studio/src/mas-fragment-editor.js
Outdated
| const masService = getService(); | ||
| masService.setAttribute('locale', Store.localeOrRegion()); | ||
| masService.activate(); | ||
| document.querySelectorAll('mas-commerce-service').forEach((el) => el.remove()); |
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.
there should be only one mas-commerce-service
| masService.setAttribute('locale', Store.localeOrRegion()); | ||
| masService.activate(); | ||
| document.querySelectorAll('mas-commerce-service').forEach((el) => el.remove()); | ||
| const masServiceElement = document.createElement('mas-commerce-service'); |
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.
instead of doing this, I recommend to cause a re-render of the shadow dom.
ideally, the locale should be a store value.
when updated, a subscription updates the mas-commerce-service
and observer mas-fragment-editor.js would re-render after it.
assuming subscription is registered earlier.
|
hi @afmicka the last issues we talked about offline are due to mismatch between what is defined in "locales", and what is defined in www/io that has partial (and sometimes different) version of possible default language, see the list: for now i see ar_MENA that should be updated to ar_SA to make things work (i tested locally) and this works, but it would not be a RCP change anymore. Up to you to wait for RCP and do the change in same PR, or to merge it now, and fix this separately |
|
@npeltier there are some conflicts. Could you please resolve them? |
|
@afmicka done! yes AR will be broken until fast follow PR. Can we make the nala test not using that (non prod yet) language for now? |
|
@npeltier i have added one Nala test for creating fragment from the scratch (covers fragment missing/undefined issue). However, this test is flaky on prod too. The same thing happens occasionally and only in automation. I am not able to reproduce it manually on prod. The difference with your branch is that the bug is there consistently and is reproducible manually every time (basically making it worse). For creating variation i need a bit of more work because i need to change the cleanup scripts too to clean those locales, beside new methods, etc. I would rather add those tests as a separate PR/jira. I am not sure i can make it on time before PTO but if i do, then you will be able to pull it in. The main issue i have is that cleanup script is not able to delete fragment in en_AU on prod (error 409). When i tried to use the force delete, then the fragment was deleted but apparently not the link somewhere in the backend. There is no variation on the main screen, there is no variation in en_AU locale either but the modal says it exists and i can't create it again (is it possible to fix it somehow now? :)
the same methods i added to this branch, i will need and use in that separate PR as well and refactor |








Resolves https://jira.corp.adobe.com/browse/MWPW-181321
QA Checklist: https://wiki.corp.adobe.com/display/adobedotcom/M@S+Engineering+QA+Use+Cases
Please do the steps below before submitting your PR for a code review or QA
🧪 Nala E2E Tests
Nala tests run automatically when you open this PR.
To run Nala tests again:
run nalalabel to this PR (in the right sidebar)To stop automatic Nala tests:
run nalalabelTest URLs: