-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Support file upload line item properties on Product Forms and Cart Notifications #745
Conversation
970b6d2
to
0d22f34
Compare
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 well!
assets/product-form.js
Outdated
const formData = new FormData(this.form); | ||
formData.append('sections', this.cartNotification.getSectionsToRender().map((section) => section.id)); | ||
formData.append('sections_url', window.location.pathname); | ||
config.body = formData; |
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 is a snippet of code above that could be moved into an else { ... }
. Since config.body
would only need to be set either as formData
or as JSON string?
if (FormData) {
...
} else {
config.body = JSON.stringify({
...JSON.parse(serializeForm(this.form)),
sections: this.cartNotification.getSectionsToRender().map((section) => section.id),
sections_url: window.location.pathname
});
}
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 point!
Your comment made me realize that serializeForm
function was already using FormData
. There's no point in keeping the two functions there, in that case. If the browser doesn't support that API, neither methods would work.
Also, my main concern with FormData
was related to specific browsers that only offer partial support for this API, like IE11 and some older mobile browsers. But I just noticed that I used formData.append
, which is supported by all of them (IE10+), and not the often incompatible formData.set
, so we should be fine :)
I removed the old implementation and the serializeForm
function and kept the new approach. Sorry for dismissing your reviews, but I believe that's a better way to handle this :DD
(please make sure re-test on multiple browser)
{%- assign property_first_char = property.first | slice: 0 -%} | ||
{%- if property.last != blank and property_first_char != '_' -%} | ||
<div class="product-option"> | ||
<dt>{{ property.first }}: </dt> |
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 wonder if we should ask the translation team about the use of :
. Does it mean the same thing everywhere ? 🤔
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 question. We actually use colon the same way in a few areas of the theme, I would be curious to know how best to handle this for i18n
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.
Yeah, that's a good question. I think the scope would be a bit broader than this PR, though. I'm just reproducing the same decision we've made for cart items here.
I'll create a separate issue so we can review this and update in multiple parts of the theme if needed.
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!
@@ -12,7 +12,7 @@ if (!customElements.get('product-form')) { | |||
onSubmitHandler(evt) { |
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.
Could be handled in a separate issue/PR - In Safari (iOS and macOS) the spinner inside the button when adding-to-cart is not visible.
I believe setting an explicit width to the svg fixes this, but requires more testing in all instances where it's being used.
.loading-overlay__spinner svg {
width: 100%;
}
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'll fix this in a separate PR. Thanks for sharing a potential fix - will test it :D
@@ -20,6 +20,23 @@ | |||
<dd>{{ option.value }}</dd> | |||
</div> | |||
{%- endfor -%} | |||
{%- for property in item.properties -%} |
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.
Since all this markup is inside the {%- unless item.product.has_only_default_variant -%}
, does that mean if the product has line item properties then item.product.has_only_default_variant
will output false
? Asking because otherwise the line item properties will not be displayed for products with only 1 variant?
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! I moved the {%- unless -%}
inside the <dl>
tag to prevent this. Should now be working for both single and multi-variant products.
sorry to chime in here since I know this is more an internal thread, but I downloaded the I note your testing directions indicate I should instead be modifying the thanks! |
Hey @M00NSH0T, what you could do is paste it in the |
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.
Looks good to me 👌
I was having some issues at first but I think it's because I needed to change the form
attribute to point for the main-product.liquid
to product-form-{{ section.id }}
sections: this.cartNotification.getSectionsToRender().map((section) => section.id), | ||
sections_url: window.location.pathname | ||
}); | ||
delete config.headers['Content-Type']; |
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.
Out of curiosity, do you mind explaining why we're removing this specific entry from the config.headers
? 😬
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.
Oh, it's because we're no longer sending a Content-Type
application/json
request. We're now using FormData
directly, without converting to JSON like we did before.
The function that returns these config headers is still being used in other endpoints across the theme, and they're still sending JSON requests, so that's why I only removed this from the /cart
request.
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.
Ah great. Thanks for the explanation 🙂
@LucasLacerdaUX While experimenting with this in Chrome, I found an odd behavior for the file. Using dawn and your test setup, the files do pass via AJAX, but if you click those same files, the file saves as a WEBP, despite the actual file extension. I disabled the AJAX, https://www.screencast.com/t/4AtCSRoVewx, and tried again with a similar file, and when going straight to cart, the file will download as it should. Firefox was not an issue. While not a big deal, it will confuse the average customer EDIT: Seems this is only an issue with PNG's |
Hey @RedPlugDesign! I think it saves as a WEBP file because of the Shopify CDN: https://cdn.shopify.com/. It automatically detects which file formats are supported on the client-side and sends the best option |
@aresena Last I tested, when the file was passed to the order admin, it saved as a WEBP, and I could not open it by simple clicking on it as a link for the order admin. Since a merchant may need to download this file via the order admin, if they could not reformat the file or download it at all, it would be an issue for them to fulfill the order. If you have a demo set up, try JPEG's & PNG's via AJAX carts and non on different browsers. You may find varying results. If you dont, great, I will have to go back and test again. Ultimate;y, I was hoping your article addressed the issue |
@RedPlugDesign True, you need to download it to see it, and it's a WEBP. Good point that some merchants could have difficulties opening it! |
Hey, its been a long time since this pull was created I was wondering if its possible to change this so the client can upload multiple files and they all show as photos separated by commas Thanks in advance |
This is a great option, thank you for this. However, once an image is uploaded to the cdn, how does the shop owner retain control over it? |
Why are these changes introduced?
Add support for
type="file"
inputs on Product Forms.Adds line item properties to cart notifications
Fixes #731
What approach did you take?
Using
FormData
, we are now able to send files to thecart_add_url
endpoint. Submitting the request asFormData
is necessary to support file uploads when adding items to cart by AJAX. Otherwise, the browser would replace theFile
object with an invalid string such asC:\fakepath\filename.png
.This is how it works:
FormData
.Content-Type
as we're no long passing ajson
as the request's body.FormData
.sections
andsections_url
.request.body
to use the FormData object.How to test 🎩
main-product.liquid
andfeatured-product.liquid
product forms:Expected results:
Demo links
Checklist