-
Notifications
You must be signed in to change notification settings - Fork 685
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
[PWA-1124] Browsing and Viewing Virtual product #3052
Conversation
… to include sub_type
…is entity attribute.
|
I have concerns around the underlying requirements for the back-end. sub_type should not be something that the store front needs to add as a requirement for the back-end nor should the implementation be tied against it. Using Custom Product Options (a default Magento Core feature) are perfectly capable of covering the use case of 1,2,3 years. It is something that
Otherwise Service really just looks like a category to me or two options taken together. |
isAddConfigurableLoading || | ||
isAddSimpleLoading, | ||
isMissingOptions || isAddConfigurableLoading || isAddSimpleLoading, | ||
isSupportedProductType, |
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.
Can list of supported product types be made a target in venia? Extensions can then add to this list.
@fooman - In full agreement here, which is why we've approached rendering this attribute as an extension. We're a bit limited by GraphQL in scope of attributes, since they currently modify the |
@tjwiebell I had hoped that new functionality was going to be built exclusively via extensions. So if you are not using virtual products you can just turn it off. Hence this PR would be the default core implementation for this. I still believe sub_type is more effort that could be spent on something that might have broader applicability (if really needed for a show case / sample implementation). and graphql provides it via something like
|
packages/extensions/venia-virtual-products/src/wrapUseCategory.js
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,25 @@ | |||
{ | |||
"name": "@magento/venia-virtual-products", |
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 this implementation is not going to be the final core implementation it would make sense to follow the sample naming convention and not use up the logical name.
"name": "@magento/venia-virtual-products", | |
"name": "@magento/venia-sample-virtual-products", |
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 call out, I've gone ahead and made this change since this is really for reference given our specific sample data.
|
Verified sub_type attribute scenarios, looks good. QA approved |
Description
As a Merchant I want to be able to showcase products that don't require delivery on PWA storefront without customization so that I decrease the time required to launch a PWA storefront and TTC of it
Preconditions
Acceptance Criteria
** product image (role: small)
** product title
** product sub-type (powered by new system attribute)
** Product price
Test Plan
** Only Virtual Product. Also check sort/filters works.
** Combination of Virtual and other Products. Also check sort/filters works.
Related Issue
Acceptance
Verification Stakeholders
Specification
Verification Steps
@magento/venia-virtual-products
to@magento/venia-concept/package.json
&&yarn
Screenshots / Screen Captures (if appropriate)
Checklist