-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Feature/2733 #3165
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
Feature/2733 #3165
Conversation
patzick
left a comment
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 branch is created from #3157, i've also changed base to develop.
Please always create branch from newest develop if you're adding features. (or newest release with stabilisation branches) read more about this here https://docs.vuestorefront.io/guide/basics/release-cycle.html
I'll hold review till #3157 is not merged.
lukeromanowicz
left a comment
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.
It's based on another task so it's hard to tell which files belong to this one and which of them don't. I migth have missed something. The code looks pretty good but again, it needs some refinements here and there
| qty: product.stock ? product.stock.qty : 0, | ||
| status: product.stock | ||
| ? product.stock.is_in_stock | ||
| ? 'ok' |
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.
could we use consts defined at the top of the file for these statuses?
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.
It seems to me that this will not improve readability in this case. Unless I do not see something;)
|
@Michal-Dziedzinski can you please apply the suggested changes and resolve conflicts in order of merging this in? |
pkarw
left a comment
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 like this change; it's using the stock cache (by using stock/list) so it's cool. As another PR you can optimize it by:
- adding loader right to the stock quantity fields while the data is loading,
- use the
volatiledata from theproduct.stock(which is cached in elastic) do display the pre-cached info while loading the dynamic stock quantity
|
@pkarw I think the loader should be a part of this PR. It's misleading yet simple to add ;) |
|
Ok! Michał can you add It Please? |
patzick
left a comment
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.
Blocked by #3157
|
@Michal-Dziedzinski @pkarw I think we should also inform users about the availability of items in group/bundle products. Now the only thing I see is inactive Add to cart button and there is no information why (e.g. For Sprite Yoga Companion Kit blundle product "Sprite Yoga Strap 8 foot" is not available and therefore the customer can not add this product to the cart) |
patzick
left a comment
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.
Just please change state modification from actions to mutations :)
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
pkarw
left a comment
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 strings needs to be passed thru i18n
patzick
left a comment
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.
Just one thing and we're good to go:) nice job!
pkarw
left a comment
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 loader should be animated gif/svg - placed on the right side of the input box; no input box name to be changed please :-)
Co-Authored-By: Patryk Tomczyk <13100280+patzick@users.noreply.github.com>
|
Looks ok on branch :) |

Related issues
closes #2733
Short description and why it's useful
I added a label to the input that tells the user how many products of a given color and size are available. You can not enter a number greater than the quantity of products . When quantity is equal to 0, the input and the button are disabled.
Screenshots of visual changes before/after (if there are any)
Before:
After:
Which environment this relates to
Check your case. In case of any doubts please read about Release Cycle
developbranch and want to merge it back todevelopreleasebranch and want to merge it back toreleasehotfixormasterbranch and want to merge it back tohotfixUpgrade Notes and Changelog
IMPORTANT NOTICE - Remember to update
CHANGELOG.mdwith description of your changeContribution and currently important rules acceptance