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

Bugfix/thumbnail images of products in view order #3273

Merged
merged 12 commits into from
Aug 26, 2019
Merged

Bugfix/thumbnail images of products in view order #3273

merged 12 commits into from
Aug 26, 2019

Conversation

jpitucha
Copy link
Collaborator

@jpitucha jpitucha commented Jul 23, 2019

Related issues

closes #2544

Short description and why it's useful

Screenshots of visual changes before/after (if there are any)

Which environment this relates to

Check your case. In case of any doubts please read about Release Cycle

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

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

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

@jpitucha
Copy link
Collaborator Author

@patzick please review

@pkarw pkarw changed the base branch from master to develop July 24, 2019 06:13
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.

Welcome @obsceniczny in our community :)
Thanks for this feature, nice start!

Things to jump more into:

  • this is feature so branch should be crated from develop and merged back to develop -> ReleaseCycle
  • instead of promises it is cleaner to use async/await; i've left you suggestions for method
  • in case of problems with loading product etc; we should show an placeholder until proper image is ready. Please get familiar with ProductImage component

Let me know if anything is unclear! :)

CHANGELOG.md Outdated
@@ -36,6 +36,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [1.10.0-rc.2] - UNRELEASED

### Added
- Added product image in order summary - @obsceniczny (#2544)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this needs to be moved into 1.11 changelog

...mapActions({
getProduct: 'product/single'
}),
getProductThumbnail (productSku) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
getProductThumbnail (productSku) {
async getProductThumbnail (productSku) {

getProduct: 'product/single'
}),
getProductThumbnail (productSku) {
if (this.itemThumbnail[productSku] === undefined || this.itemThumbnail[productSku] === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it is the same as this, but more readable

Suggested change
if (this.itemThumbnail[productSku] === undefined || this.itemThumbnail[productSku] === null) {
if (!this.itemThumbnail[productSku]) {

}),
getProductThumbnail (productSku) {
if (this.itemThumbnail[productSku] === undefined || this.itemThumbnail[productSku] === null) {
this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false, skipCache: true })
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false, skipCache: true })
const product = await this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false })

getProductThumbnail (productSku) {
if (this.itemThumbnail[productSku] === undefined || this.itemThumbnail[productSku] === null) {
this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false, skipCache: true })
.then(product => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
.then(product => {

this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false, skipCache: true })
.then(product => {
this.itemThumbnail[productSku] = getThumbnailPath(product.image, 80, 80)
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
})

if (this.itemThumbnail[productSku] === undefined || this.itemThumbnail[productSku] === null) {
this.getProduct({ options: { sku: productSku }, setCurrentProduct: false, setCurrentCategoryPath: false, selectDefaultVariant: false, skipCache: true })
.then(product => {
this.itemThumbnail[productSku] = getThumbnailPath(product.image, 80, 80)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.itemThumbnail[productSku] = getThumbnailPath(product.image, 80, 80)
this.itemThumbnail[productSku] = getThumbnailPath(product.image, 80, 80)

@@ -63,35 +66,38 @@
<td class="fs-medium lh25" :data-th="$t('Subtotal')">
{{ item.row_total_incl_tax | price }}
</td>
<td class="fs-medium lh25">
<img :src="getProductThumbnail(item.sku)">
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would be nice to use ProductImage component here, to show placeholders until proper product image is loaded

@jpitucha
Copy link
Collaborator Author

@patzick could You look?

@patzick
Copy link
Collaborator

patzick commented Jul 25, 2019

Thanks @obsceniczny, i need to checkout this branch and test if there is placeholder before actual image is loaded. Overall looks good now, i will get back with this soon :)

patzick and others added 3 commits July 25, 2019 15:24
@jpitucha
Copy link
Collaborator Author

@patzick I fixed linting causing Travis failing

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.

You're doing well, we're near the end. I've noticed this skipGrouped method and it would be cool to refactor it with this PR. Also it would be great to have a bigger images, because they are a little bit blurry

image

getProduct: 'product/single'
})
},
beforeMount () {
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 do that on mounted, so there is no way to have problems with SSR

Suggested change
beforeMount () {
mounted () {

})
},
beforeMount () {
this.skipGrouped(this.order.items).forEach(async item => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you also with this PR change skipGrouped method to computed singleOrderItems or something like this? It should really be a computed property not a method, and then here (and in other places) it would just be this.singleOrderItems.find(...

Copy link
Collaborator Author

@jpitucha jpitucha Aug 2, 2019

Choose a reason for hiding this comment

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

I don't really see what do you mean by using find. We process a whole array here, we're not aiming at getting a single element, could you elaborate? As for computed property I completely agree.

},
beforeMount () {
this.skipGrouped(this.order.items).forEach(async item => {
if (this.itemThumbnail[item.sku] === undefined || this.itemThumbnail[item.sku] === null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's the same as

Suggested change
if (this.itemThumbnail[item.sku] === undefined || this.itemThumbnail[item.sku] === null) {
if (this.itemThumbnail[item.sku]) {

@pkarw
Copy link
Collaborator

pkarw commented Jul 31, 2019

@obsceniczny I just wanted to kindly ask you about the progress with this ticket? Can we apply the changes sugessted by @patzick in order to merge this in?

@jpitucha
Copy link
Collaborator Author

@pkarw I have some uncommited changes and I will do my best to end this until weekend

@patzick
Copy link
Collaborator

patzick commented Aug 8, 2019

@obsceniczny is it ready for re-review? :)

@jpitucha
Copy link
Collaborator Author

jpitucha commented Aug 8, 2019

Yes, it is ;)

@pkarw pkarw requested a review from patzick August 19, 2019 07:12
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.

Thanks, @obsceniczny, nice work! :)

@patzick patzick merged commit 09d9f8a into vuestorefront:develop Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants