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

Change product color or size when product is in the cart #3090

Conversation

andrzejewsky
Copy link
Contributor

Related issues

closes #2346

Short description and why it's useful

Hello VS team!
I've been learning Vue.js recently and this PR is my very first contact of it (besides some small things). I introduced possibility to change the color or size of the product that is in the cart. I spent some time with acquainting of this product and how it works and... I hope i've done everything correctly because I had many doubts about API, what should I use etc. Please have look at it.

Cheers 🎉

Screenshots

Screenshot 2019-06-20 at 22 12 37

Which environment this relates to

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch 3 times, most recently from 2e14523 to ced8c1e Compare June 20, 2019 21:20
@filrak filrak requested review from filrak, patzick and pkarw and removed request for filrak June 21, 2019 16:09
@filrak
Copy link
Collaborator

filrak commented Jun 21, 2019

Hello! This is a long-requested feature, thanks for proposing it! I'm leaving the review to @patzick who has recently worked with cart module

@filrak filrak removed their request for review June 21, 2019 16:10
@pkarw
Copy link
Collaborator

pkarw commented Jun 22, 2019

Thanks! I like the implementation the only question is the UI - not sure about it probably needs to be redesigned - @alinadivante can you take a look and consult it with our UX team please?

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

I’ve requested some changes plus we need to improve the UX :-) at least make the controls a little bit smaller :D

@@ -295,7 +296,7 @@ const actions: ActionTree<CartState, RootState> = {
continue
}
if (config.entities.optimize && config.entities.optimizeShoppingCart) {
product = omit(product, ['configurable_children', 'configurable_options', 'media_gallery', 'description', 'category', 'category_ids', 'product_links', 'stock', 'description'])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change will increase the size of the order json passed to server endpoint api/order/create. We should remove these properties in order/placeOrder action just before passing them to the server

@@ -387,6 +388,20 @@ const actions: ActionTree<CartState, RootState> = {
return diffLog
}
},
updateVariant (context, { product, configuration }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename it to “configureItem” to make it coherent with “addIten”, “renoveItem” and also with “product/configure” actiona

core/modules/cart/store/actions.ts Show resolved Hide resolved
@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from 2b119b5 to be23e22 Compare June 22, 2019 15:45
@pkarw
Copy link
Collaborator

pkarw commented Jun 22, 2019

Thanks for the comments - could you update the code regarding the other comments posted?

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from be23e22 to 72d7168 Compare June 22, 2019 15:53
@andrzejewsky
Copy link
Contributor Author

@pkarw Just posted everything (look at file diff tab, squashed commits to just one), this is how it looks now:

Screenshot 2019-06-22 at 17 46 01

@pkarw
Copy link
Collaborator

pkarw commented Jun 22, 2019

Cool. Regarding the UX - our design team will probably propose something, But wouldn't it be cool to have the controls much smaller and put along side the selected options?

Color: Blue [ ] [ ] [ ] [x] [ ] -> so you can switch the color right next to the selected onel the controls should be in that case like 8px in height.

I think that it would be also cool to have this feature as an option (enabled by default in the config.cart.productsAreReconfigurable

@@ -19,6 +20,7 @@ const actions: ActionTree<OrderState, RootState> = {
* @param {Order} order order data to be send
*/
async placeOrder ({ commit, getters, dispatch }, order: Order) {
order.products = order.products.map(product => omit(product, ['configurable_options', 'configurable_children'])) as Order['products']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please just add the check if (config.entities.optimize && config.entities.optimizeShoppingCart) {

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from 72d7168 to 9ec8378 Compare June 22, 2019 17:43
@andrzejewsky
Copy link
Contributor Author

@pkarw Update:

Screenshot 2019-06-22 at 19 45 21

@alinadivante
Copy link
Collaborator

Thank you for your suggestion @andrzejewsky ! :)
I think that Size can be displayed as drop-down list, but I will consult UX solution with @StasiekDivante

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from 9ec8378 to 175416f Compare June 25, 2019 08:21
@patzick patzick added the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jun 28, 2019
@StasiekDivante
Copy link
Collaborator

StasiekDivante commented Jul 1, 2019

Screenshot 2019-07-01 at 13 13 29
@alinadivante to be honest I don't think people will use color / size change at cart page that often so we shouldn't risk complicating UI for the sake of usability here. I would go with simple "Edit" option next to "remove"

@alinadivante
Copy link
Collaborator

@StasiekDivante thanks for your feedback.
My question is: will Edit activate the possibility of changing both parameters? or how will it work?

@StasiekDivante
Copy link
Collaborator

StasiekDivante commented Jul 1, 2019

Sure, edit would activate possibility to edit both options, it would look something like this:

Screenshot 2019-07-01 at 13 25 19

background would dim and this position would light up, with full-size color and size picker and CTA for updating the item

PS Please check how such a functionality works on www.adidas.com to get a grasp of that it might look like. (go to adidas.com, add product to cart, enter cart, click on edit in the product)

@alinadivante
Copy link
Collaborator

@StasiekDivante interesting solution :)
what do you think @pkarw? In my opinion, it's fine!

@pkarw
Copy link
Collaborator

pkarw commented Jul 1, 2019

Looks great! @andrzejewsky can you please change the UI regarding this design? Please do also resolve the conflicts in thie core/pages/Product.js

@andrzejewsky
Copy link
Contributor Author

@pkarw yeah, will be working on it this week.

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from 175416f to 6538835 Compare July 4, 2019 15:05
@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from d8a855a to 9c0754e Compare July 13, 2019 17:00
@andrzejewsky
Copy link
Contributor Author

Updated. I've made a little bit refactoring because of lack of possibility to fetch parent product when I have only simple one. As the result catalog/store/product/actions.ts is smaller, because I extracted some logic from list action to helpers. Now two new (reusable) functions are available:

findProductsByQuery - it was used in the list action for loading products in the catalog
findConfigurableParent - for loading parent product (+configured if configuration is passed)

createProductListHelpers - is just a extracted logic from list action

Now, editing mode works even with the simple products. When you click "Edit" the configurable parent will be loaded.

Also I covered the case when there are two products with the same sku - it's possible because in the edit mode you can configure the same filters as some product in the cart already has. In that case the next product with same sku will be deleted and the edited one will increase quantity by the quantity of deleted one.

@pkarw pkarw requested a review from patzick July 13, 2019 19:34
@pkarw
Copy link
Collaborator

pkarw commented Jul 13, 2019

Good job with the refactoring! @patzick just takie a look - product/list just got much much clear and it’s something that might be pretty useful for you for the Product.vue refactoring

@@ -19,6 +20,9 @@ const actions: ActionTree<OrderState, RootState> = {
* @param {Order} order order data to be send
*/
async placeOrder ({ commit, getters, dispatch }, order: Order) {
if (config.entities.optimize && config.entities.optimizeShoppingCart) {
order.products = order.products.map(product => omit(product, ['configurable_options', 'configurable_children'])) as Order['products']
Copy link
Collaborator

Choose a reason for hiding this comment

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

Related to: #3222

@pkarw pkarw added QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. and removed QA rejected Testers will add this label when something is still wrong labels Jul 14, 2019
@alinadivante
Copy link
Collaborator

Fixed :)
image

@alinadivante alinadivante added QA approved on branch Testers will add this label after positive check on specific branch. and removed QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. labels Jul 15, 2019
@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from 2df8d07 to b8a5596 Compare July 15, 2019 12:58
Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Mostly minor changes, but i believe thing with helpers without vuex context is really important thing.
Please guys give me your thoughts.

CHANGELOG.md Outdated
@@ -72,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Products column change functionality - @vishal-7037 (#3017)
- New Module order-history this provides the pagination via lazy laod - @hackbard (#2810)
- OrderNumber on ThankYouPage - @Flyingmana (#2743)
- Added possibility to change color or size of the product that is already in the cart - @andrzejewsky (#2346)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is PR to develop, so to newest version. You added it to 1.10 which was already released.
Please change it to 1.11 and read about Releasing cycle

return product
}

const configureTaxes = (context, { resp, isCacheable, cacheByKey }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

basically if helper needs a vuex context, then it should be an action not helper. Otherwise helpers instead of helping they are distracting vuex logic to parts.
Helpers should do computations etc. - take data and return result, which is also easy to test by unit tests.
But vuex actions should be the ones, which are dispatching other actions.
Let me know if you understand this point of view - it is really important to have this understanding of helpers.
cc @pkarw @filrak @przspa @lukeromanowicz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patzick completely agree, basically I wanted to ask about this point just to clarify what helpers means in this project. My understanding was exactly like yours but I saw context-bound in the other helpers so I thought that sometimes you are using it in that way and that's allowed. Ok I'll try to refactor it as much as it is possible, because probably some of them would be useful later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, we have that kind of helpers, but we're trying to refactor them in time with this conception. That's cool that you see this the same way. I agree that some of them are reusable, but only with logic part - with every dispatch there should be an action, which can also be reusable if is doing one specific thing at the time not set of things :)

@@ -0,0 +1,10 @@
export interface Configuration {
Copy link
Collaborator

Choose a reason for hiding this comment

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

i'd change it to ProductOption. Configuration is more reserved to application Configuration.


const actions = {
configureProduct (context, { product }) {
return new Promise((resolve, reject) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use async/await instead of Promise syntax

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch 2 times, most recently from 14eba3d to 56fde70 Compare July 16, 2019 20:29
@andrzejewsky
Copy link
Contributor Author

@patzick I applied your comments + refectored actions/helpers - everything that I could, was extracted to the helpers and one big list action was split to the smaller ones. I think some of them could be refactored even more + would be nice to have helpers tested, but I think maybe it's something for different taks because this feature becoming gradually more complex.

@pkarw pkarw removed the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Jul 17, 2019
@pkarw
Copy link
Collaborator

pkarw commented Jul 17, 2019

@patzick for me it's ready to be merged in :)
@andrzejewsky - great job

@andrzejewsky andrzejewsky force-pushed the feature/2346-change-color-or-size-in-cart branch from bb45390 to f9b37fb Compare July 18, 2019 08:50
@andrzejewsky
Copy link
Contributor Author

Any update of that? Could it be merged? Because it's only waiting and I have to resolve conflict sometimes.

Copy link
Collaborator

@patzick patzick left a comment

Choose a reason for hiding this comment

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

Everything looks good, thanks @andrzejewsky ! :)

@patzick patzick added this to the 1.11.0-rc.1 milestone Jul 18, 2019
@patzick patzick merged commit f273b13 into vuestorefront:develop Jul 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA approved on branch Testers will add this label after positive check on specific branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants