-
Notifications
You must be signed in to change notification settings - Fork 132
Conversation
So far this looks great! Thanks for getting started on it. Having a standardized (or close to it) way of handling product forms will help tremendously with all the Shopify Apps that like to hijack or take a ride on the variant, product and cart events. Especially when handling the Print on Demand Clothing
Phone Cases
Right now most variant code, including Slate, has always assumed that every possible combination is a valid variant—3 types * 2 colors * 4 sizes = 24 total. In reality, many shops and products present gaps in the valid option combinations. The thought process is that the selection of variants should do two things... 1) Update the availability (in stock / out of stock) of the other variants in the other options. 2) Hide/show other valid variant combinations in the other options. For all the stores that use options like this, it causes quite a lot of confusion for their customers if a product is simply out of stock or if it's a combination that has never existed. At the very least, it would be helpful to update data attributes on the radio items that are selected, valid and available. Then it could be up to the theme's design and interpretation to decide how
|
@jonathanmoore thanks for this. This package is the perfect place to land on a standard approach for these situations. Starting to dive into your scenarios now. |
I'm now wondering if this package is the best place to handle these scenarios... Product option selection ends in three results: Available, Sold Out, and Invalid Combination. This library currently handles all three of those results. One of the biggest differences between If the theme developer does not wish the user to select Sold Out or Invalid combinations, it's the rendering logic of the page that should prevent those options from being selected. That being said, you present a very real problem that theme developers encounter when rendering a product form. I think @shopify/theme-product would be a suitable place for us to help theme developers with the confusing logic behind enabling/disabling and hiding/showing option selections based on variant availability, i.e. this sounds like more of a problem with navigating and computing the Product object. I'm feeling comfortable that we can tackle this problem in a separate scope from this PR. Could you please make a new issue with the problem you just discussed? I'd still like to tackle it and get something in Shopify Starter Theme as soon as possible. |
cd827c9
to
79aff60
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.
The documentation looks really good and the explanation as well 👍
I only read the tests and documentation. I will check the script later on.
packages/theme-product-form/__tests__/theme-product-form.test.js
Outdated
Show resolved
Hide resolved
packages/theme-product-form/__tests__/theme-product-form.test.js
Outdated
Show resolved
Hide resolved
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'm really liking the implementation of this so far. I still need to look through the tests and actually test the library, but wanted to post some preliminary feedback.
"rollup-plugin-uglify": "^4.0.0", | ||
"size-limit": "^0.18.3" | ||
}, | ||
"size-limit": [ |
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.
packages/theme-product-form/__tests__/theme-product-form.test.js
Outdated
Show resolved
Hide resolved
packages/theme-product-form/__tests__/theme-product-form.test.js
Outdated
Show resolved
Hide resolved
a6ecee4
to
22489f0
Compare
22489f0
to
56443f7
Compare
56443f7
to
2f10d29
Compare
@@ -6,7 +6,7 @@ | |||
"main": "index.js", | |||
"scripts": { | |||
"test": "jest", | |||
"bootstrap": "yarn && lerna bootstrap", | |||
"bootstrap": "yarn && lerna bootstrap && lerna run build", |
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.
🎉
Fixes #44
Contributes towards Shopify/starter-theme#140
This package is made to replace @shopify/theme-variants. It's purpose is to help developers create product forms. More details to come.
TODO:
radio
andcheckbox
s@chrisberthe @martinamarien @tauthomas01 I have requested your review as this will package is destined for your project and I would love to get as many 👀 on it as possible. Feel free to focus more on the overall architecture described in the README if time is limited for reviewing. Also use Shopify/starter-theme#142 as example of how the package will be used.