-
Notifications
You must be signed in to change notification settings - Fork 615
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
CATALOG-2408 - Bulk Pricing updates per-variant #1167
Conversation
Autotagging @bigcommerce/storefront-team @davidchin |
package.json
Outdated
@@ -6,7 +6,7 @@ | |||
"author": "BigCommerce", | |||
"license": "MIT", | |||
"dependencies": { | |||
"@bigcommerce/stencil-utils": "^1.0.10", | |||
"@bigcommerce/stencil-utils": "bookernath/stencil-utils#d263f2ef03aa88e1361c8b4331204e4e4e8bd5e4", |
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.
this gets replaced with a new version once the utils PR is merged.
assets/js/theme/cart.js
Outdated
@@ -98,7 +98,7 @@ export default class Cart extends PageManager { | |||
const $messageBox = $('.alertMessageBox'); | |||
const item = $('[name="item_id"]', $form).attr('value'); | |||
|
|||
utils.api.productAttributes.optionChange(item, $form.serialize(), (err, result) => { |
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 at the new optionChange
api, it seems that we don't need to pass null
here
const productAttributesData = response.data || {}; | ||
|
||
const bulkPricingHtml = response.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.
could we match the productAttributesData here, like so:
const productAttributesContent = response.content || {};
...
updateView(data, content)
etc.
@@ -393,6 +393,13 @@ export default class ProductDetails { | |||
viewModel.$addToCart.prop('disabled', false); | |||
viewModel.$increments.prop('disabled', false); | |||
} | |||
|
|||
// If Bulk Pricing rendered HTML is available | |||
if (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.
does this work? i thought it was grabbing a field off of content... like this instead:
updateView(data, content) { // content will be {} if response.content is empty above
...
if (content.bulkPricingContent) { // or whatever it's called
viewModel.$bulkPricing.html(content.bulkPricingContent)
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.
Content returns a blank string which evaluates to false if there are no tiers. Content is returned as another top-level property, but it's just a string and not an object.
@mjschock I think I've addressed most of your feedback if you wanna take a peek. Also rebased to pull in the JQ update. |
What?
Price Lists support different bulk pricing tiers for each variant, so we need to update the viewModel whenever these tiers change.
Tickets / Documentation
Screenshots (if appropriate)