-
Notifications
You must be signed in to change notification settings - Fork 0
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
PDP Hero Section Updates, Pt 2 #174
Conversation
🤖 [themegit] The theme preview URL for 63e7bf3 is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
position: relative; | ||
|
||
&::after { | ||
content: "▼"; |
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.
Currently using this unicode character but will confirm with XXIX if we can use this or need to use the svg triangle from Figma.
🤖 [themegit] The theme preview URL for aedcf7c is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
🤖 [themegit] The theme preview URL for eeb9b28 is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
🤖 [themegit] The theme preview URL for 2fa493f is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
06cfa1e
to
68292bb
Compare
🤖 [themegit] The theme preview URL for 7a8fb19 is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
<option value="" selected="selected">{{"snippets.add_to_cart_container.quantity_spinner.label" | t }}</option> | ||
{% comment %} | ||
TO-DO: Add notification if user adds quantity > product inventory | ||
{% endcomment %} |
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.
@sepowitz and I talked about adding this to-do in the case that the added quantity is > product inventory, so the user is notified that they can't add X quantity.
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.
Are you able to use liquid to get the available inventory and just disable Quantity options that exceed that amount?
const addToCartButton = document.querySelector(Cart.SELECTORS.addToCartButton); | ||
|
||
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.
Added a to-do to update this function. This was written when products only had one variant.
- When the user adds an item to their cart, if the item in the cart has the same
variantId
as the product page you are currently on, theAdd to Cart button
changes to "Added to cart" - This no longer works now that we have products with several variants.
- In a future PR, I will update this function to check when the
Add to cart button
is clicked and use asetTimeout
to change the text to "Added to cart" for a few seconds before reverting back to "Add to cart" so users can add multiple variants to their cart.
🤖 [themegit] The theme preview URL for f987a52 is now deployed here: https://nunu-school.myshopify.com/?preview_theme_id=122395525300 |
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.
Looking good! My comments are from the POV of someone who has never seen this code before -- there's just some things happening in cart.ts
that I had to dig into a bit to understand. So just some suggestions on improving that, but also --- I haven't had to do a vanilla JS + liquid thing in a long time, I remember how annoying it was and do not envy you...
{% for i in (1...10) %} | ||
<option value="{{ i }}">{{ i }}</option> | ||
{% endfor %} |
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.
Is there a reason why the quantity is a select from 1-10 and not a number input?
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.
Gooood question! It was originally a number input but I changed it to a <select>
because the design is a dropdown menu! When I looked into it, it seems like the way to create that design is a <select>
. What do you think?
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.
If that's what is designed, then yeah a dropdown makes sense! The only issue I can imagine is if there is someone who wants to add more than 10... but I don't think this will come up and they can just refresh and add in multiple batches i spose
@@ -216,7 +216,7 @@ export default(function() { | |||
} |
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.
For lines 210-216 (github won't actually let me comment on lines that are collapsed in the PR review zone)
These two calls might be able to be simplified:
from:
const cartHtml = await Cart.getCartHtml();
Cart.render(cartHtml);
const cartItemCount = document.querySelector(Cart.SELECTORS.cartItemCount).getAttribute(Cart.ATTRIBUTES.itemCount);
if (cartItemCount) {
Cart.updateCartItemCount(cartItemCount);
}
to simply:
await Cart.render()
Cart.updateCartItemCount()
Then let these two functions deal with getting the cartHtml
and the cartItemCount
, respectively
const addToCartButton = document.querySelector(Cart.SELECTORS.addToCartButton); | ||
|
||
if (!addToCartButton) return; | ||
|
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.
Again my comment is for a line that is out of reach...
The fact that you're calling Cart.init(false) at the end of this function is a smell to me - shouldn't it already have been initialized? Maybe separate out the functionality that binds the addToCart forms into a separate method and call that here instead?
If I were to read Cart.bindAddToCartForms()
at the bottom of this function I'd have a better sense of what's going on.
@@ -285,9 +285,10 @@ export default(function() { | |||
}) |
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.
Line 270 above:
I hadn't ever done this: [].slice.call(...)
- it looks like it's just to convert a NodeList into an array? You can use Array.from(document.querySelectorAll(...))
. Or better yet just make a utility function:
function getElements(selector: string): Element[] {
return Array.from(document.querySelectorAll(selector))
}
Then your usage of it will be a little more semantic
@@ -216,7 +216,7 @@ export default(function() { | |||
} | |||
|
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.
Line 209, above: would it be worth adding an Action.UPDATE
and making the action argument required?
(it looks like it's required in handleAddtoCartButton
... since it's optional in onChange
I'd expect TS to be bugging you about this?)
<option value="" selected="selected">{{"snippets.add_to_cart_container.quantity_spinner.label" | t }}</option> | ||
{% comment %} | ||
TO-DO: Add notification if user adds quantity > product inventory | ||
{% endcomment %} |
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.
Are you able to use liquid to get the available inventory and just disable Quantity options that exceed that amount?
Previously I was getting the It also gets complicated when the user selects one variant and then selects another. The quantity options should update based on the selected variant. @sepowitz mentioned that to implement this, it would be a bit more complex (e.g. have eventListeners listening to changes in the variant dropdown menu). I have about a week left, not full time, to make as many design updates as possible. So for lack of time we decided to go with having the quantity options as 1 - 10 to keep things simple. Then in the future, either show a notification if user adds quantity > product inventory, or refactor to only show quantity options <= product inventory. Maybe this could be something for Udit to work on if he's interested? |
@good-idea thank you for taking the time to review, especially the cart! I'm going to take these comments and make an issue so we can make improvements to the cart in a separate PR. :) ETA: Issue #176 |
Description
ProductTextFields
, which are repeatable Accentuate text fields for any text + wingdings the editors want to add (design)NOTE: For now, ignore
themegit
and check this preview.Type(s) of changes
Motivation for PR
#166, and part 2 of #173
How Has This Been Tested?
Preview: https://qmvt59fhb48l21hh-52674822324.shopifypreview.com
Example of product with variants:
https://index-space.org/products/anthropocene-copy
Applicable screenshots:
Follow-up PR
cart.ts
: This function should check when the add to cart button is clicked and use a setTimeout to change the text to "Added to cart" and then revert back to "Add to cart" after a few seconds.