Skip to content
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

CIF-1473: Support Bundle Products: option selection #378

Merged
merged 25 commits into from
Sep 4, 2020
Merged

Conversation

laurentiumagureanu
Copy link
Collaborator

Description

ProductOptionsBundle react component will be mounted in a placeholder div element that is present for bundle products.
Initially displays a Customize button. Once the button is pressed, it performs a graphQL query to fetch the bundle options and renders it accordingly.

The add to cart is not implemented as there's a separate issue for it.

Related Issue

CIF-1473

Motivation and Context

Support Bundle Products

How Has This Been Tested?

Unit tests; manually

Screenshots (if appropriate):

image
image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #378 into master will increase coverage by 3.47%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #378      +/-   ##
============================================
+ Coverage     85.09%   88.57%   +3.47%     
  Complexity      924      924              
============================================
  Files           172       83      -89     
  Lines          4174     3046    -1128     
  Branches        659      290     -369     
============================================
- Hits           3552     2698     -854     
+ Misses          485      211     -274     
  Partials        137      137              
Flag Coverage Δ Complexity Δ
#integration 68.20% <ø> (+0.03%) 687.00 <ø> (+1.00)
#jest ? ?
#karma 94.32% <ø> (?) 0.00 <ø> (?)
#unittests 86.07% <ø> (ø) 892.00 <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ts/src/components/ForgotPassword/forgotPassword.js
...mponents/src/components/CartTrigger/cartTrigger.js
.../components/Checkout/paymentProviders/braintree.js
...mponents/src/components/Checkout/checkoutButton.js
react-components/src/components/Portal/Portal.js
...components/src/components/Checkout/paymentsForm.js
...omponents/src/components/Minicart/useCouponForm.js
...act-components/src/components/Minicart/minicart.js
react-components/src/components/Minicart/footer.js
...act-components/src/utils/transparentPlaceholder.js
... and 98 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62ed3f...1371808. Read the comment docs.

Copy link
Member

@herzog31 herzog31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should rethink, how you use CSS in this component. Yes it makes sense to use the classes from the server-side components, but on the other hand, it messes with the customisation patterns of the React components. @dplaton wdyt?

</div>
</section>
<section className="productFullDetail__cartActions productFullDetail__section">
<button
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The event handler on the add to cart button via https://github.com/adobe/aem-core-cif-components/blob/master/ui.apps/src/main/content/jcr_root/apps/core/cif/components/commerce/product/v1/product/clientlib/js/addToCart.js is only done once for when the whole document is loaded. Since this component starts with an initial bundleState = null, the button is not rendered and the event handler should not be set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the intention is to not use the default event handler for adding to cart since a bundle requires a different mutation.
I can modify the classes to avoid any confusion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that would mean copying the CSS rules from productFullDetail__cartActions to another class and use that one instead in this button. not sure it's worth it

@dplaton
Copy link
Contributor

dplaton commented Aug 17, 2020

I'm fine with the whole code (I have more or less the same comments as @herzog31), but I have a problem with the whole approach: all our "React Core Components" are standalone. They do share context, but you can use the shopping cart without the authentication part, and you can use the whole My Account suite of components without the Shopping Cart.

However, this feature is tied to a Product component. Even more, this component doesn't even share state with the other components, it's just used for a specific flavor of a product component. With this in mind, I'm really wondering if we should even have this component as part of the React Core Components project. I can think of two other options:

1/ Have it as a React component, but part of ui.apps (like we used to have the "Venia Demo")
2/ Just have it in plain JS that uses the GraphQL API, just like the price component.

@laurentiumagureanu
Copy link
Collaborator Author

I'm fine with the whole code (I have more or less the same comments as @herzog31), but I have a problem with the whole approach: all our "React Core Components" are standalone. They do share context, but you can use the shopping cart without the authentication part, and you can use the whole My Account suite of components without the Shopping Cart.

However, this feature is tied to a Product component. Even more, this component doesn't even share state with the other components, it's just used for a specific flavor of a product component. With this in mind, I'm really wondering if we should even have this component as part of the React Core Components project. I can think of two other options:

I agree that having this one functionality as part of React Core Components is a bit overkill and the use frequency will be low.

1/ Have it as a React component, but part of ui.apps (like we used to have the "Venia Demo")

I think I can just create a React app as part of the Product clientlib and have it mounted just for Bundle Products

2/ Just have it in plain JS that uses the GraphQL API, just like the price component.

Having this implemented as plain Javascript can be hard to do and maintain, the main issue being the HTML rendering (which can easily become spaghetti code).

@herzog31
Copy link
Member

IMO having yet another React project in our repos is overkill. I'm completely fine with having this feature as part of the react-components project. Maybe you could refactor the way the component gets the sku, so it could be (unlikely) used in other contexts as well.

@dplaton
Copy link
Contributor

dplaton commented Aug 17, 2020

I think I can just create a React app as part of the Product clientlib and have it mounted just for Bundle Products

That's a good idea since the client libraries already get built with Webpack, so no need to create another React project. The only downside is that you'd have to re-write your tests.

IMO having yet another React project in our repos is overkill.

That's my thinking as well, @herzog31, hence the idea above.

@herzog31 herzog31 added the feature New feature or request label Aug 25, 2020
Copy link
Member

@herzog31 herzog31 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like so see some tests for all the onX methods of the input fields (checkbox, multiselect, radio etc).

@LSantha LSantha merged commit 68faa85 into master Sep 4, 2020
@LSantha LSantha deleted the issues/CIF-1473 branch September 4, 2020 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants