-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 pickup-in-store international domains and multi-language support #848
Conversation
80c512b
to
eeacde8
Compare
if (!rootUrl.endsWith("/")) { | ||
rootUrl = rootUrl + "/"; | ||
} | ||
const variantSectionUrl = `${rootUrl}variants/${variantId}/?section_id=pickup-availability`; | ||
|
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.
Works great from what I can tell. I noticed when testing this fix on the store mentioned in the issue that there is a conditional we need to add in order to display the proper information on the storefront.
In the renderPreview()
function, we need to check if the button exists first before this.querySelector('button').addEventListener...
Maybe we can add an early return before if the button doesn't exist:
if (!this.this.querySelector('button')) return;
Let me know if you're ok with adding this fix in this PR as well, otherwise I can create a follow up issue and PR 👍
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.
Good catch
Let me know if you're ok with adding this fix in this PR as well, otherwise I can create a follow up issue and PR
I think since it is unrelated to the specific of the issue being solved here, I would prefer to have this shipped and iterated over.
I would also appreciate since you are offering to have you take over this specific change. I'm not familiar with the specifics of the JS enhancements in Dawn(in themes in general) to be confident as to the blast radius here.
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.
Just for sake of visibility, I've created the follow up PR about this issue here: #855
But nothing is needed, I requested reviews from my teammates.
Why are these changes introduced?
Fixes #548
As per the issue description, we must always use the current domain to issue an ajax call or otherwise a CORS issue will occur.
Additionally, the concatenation was not valid for multi-language use cases where the resulting url was not valid.
Demo links
URL: https://jp.defaultglobal.com/products/blue-silk-tuxedo
Generated XHR: https://defaultglobal.com/variants/31362180874262/?section_id=pickup-availability
Result:
URL: https://defaultglobal.com/en-fr/products/blue-silk-tuxedo
Generated XHR: https://defaultglobal.com/en-fr/en-frvariants/31362180874262/?section_id=pickup-availability
Result:
Checklist