-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Change product color or size when product is in the cart #3090
Conversation
2e14523
to
ced8c1e
Compare
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 |
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? |
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’ve requested some changes plus we need to improve the UX :-) at least make the controls a little bit smaller :D
core/modules/cart/store/actions.ts
Outdated
@@ -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']) |
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 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
core/modules/cart/store/actions.ts
Outdated
@@ -387,6 +388,20 @@ const actions: ActionTree<CartState, RootState> = { | |||
return diffLog | |||
} | |||
}, | |||
updateVariant (context, { product, configuration }) { |
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.
Rename it to “configureItem” to make it coherent with “addIten”, “renoveItem” and also with “product/configure” actiona
src/themes/default/components/core/blocks/Microcart/Product.vue
Outdated
Show resolved
Hide resolved
2b119b5
to
be23e22
Compare
Thanks for the comments - could you update the code regarding the other comments posted? |
be23e22
to
72d7168
Compare
@pkarw Just posted everything (look at file diff tab, squashed commits to just one), this is how it looks now: |
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 |
core/modules/order/store/actions.ts
Outdated
@@ -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'] |
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.
Please just add the check if (config.entities.optimize && config.entities.optimizeShoppingCart) {
72d7168
to
9ec8378
Compare
@pkarw Update: |
Thank you for your suggestion @andrzejewsky ! :) |
9ec8378
to
175416f
Compare
|
@StasiekDivante thanks for your feedback. |
Sure, edit would activate possibility to edit both options, it would look something like this: 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) |
@StasiekDivante interesting solution :) |
Looks great! @andrzejewsky can you please change the UI regarding this design? Please do also resolve the conflicts in thie |
@pkarw yeah, will be working on it this week. |
175416f
to
6538835
Compare
d8a855a
to
9c0754e
Compare
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
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 |
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'] |
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.
Related to: #3222
2df8d07
to
b8a5596
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.
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) |
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 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 }) => { |
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.
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
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.
@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.
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.
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 { |
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'd change it to ProductOption. Configuration is more reserved to application Configuration.
|
||
const actions = { | ||
configureProduct (context, { product }) { | ||
return new Promise((resolve, reject) => { |
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.
please use async/await instead of Promise syntax
14eba3d
to
56fde70
Compare
@patzick I applied your comments + refectored actions/helpers - everything that I could, was extracted to the helpers and one big |
@patzick for me it's ready to be merged in :) |
bb45390
to
f9b37fb
Compare
Any update of that? Could it be merged? Because it's only waiting and I have to resolve conflict sometimes. |
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.
Everything looks good, thanks @andrzejewsky ! :)
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
Which environment this relates to
Upgrade Notes and Changelog
Contribution and currently important rules acceptance