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

The product image updates when a characteristic is selected. #760

Merged
merged 13 commits into from
Jul 12, 2019

Conversation

devbucket
Copy link
Contributor

Description

Instead of waiting for the variant selection to be completed, the product image now updates when a single characteristic, like e.g. the color, is selected.

Type of change

Please add an "x" into the option that is relevant:

  • Bug Fix 🐛 (non-breaking change which fixes an issue)
  • Enhancement 🚀 (non-breaking change which adds functionality)
  • Breaking Change 💥 (fix or feature that would cause existing functionality to not work as expected)
  • Polish 💅 (Just some cleanups)
  • Internal 🏠 Only relates to internal processes.

How to test it

Please describe here any specialty that the tester should be aware of.

@devbucket devbucket added the enhancement New feature or request label Jul 8, 2019
@devbucket devbucket self-assigned this Jul 8, 2019
}

/**
* Creates a selector that retrieves the featured image URL for a selectred characteristic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Type selectred

const Media = ({
'aria-hidden': ariaHidden, productId, variantId, imageUrl,
}) => {
if (imageUrl) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whenever the an imageUrl is available a ProductImage is rendered, instead of the MediaSlider / ImageSlider. Since all products from the variants provide an imageUrl, the galleries will not be rendered anymore.

Copy link
Collaborator

@fkloes fkloes Jul 10, 2019

Choose a reason for hiding this comment

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

What i meant with this comment is, that the won't be image sliders anymore when the code would be merged.

// imageUrl will always have a value after the very first characteristic value was selected
if (imageUrl) {
    return (<ProductImage />)
}
// Will never be rendered again for child products
return (<ImageSlider />)

With your latest changes you link this product image to the first image of the parent product. So if one clicks the product image, there is a high chance, that a wrong image is presented on the gallery page.

makeGetProductByCharacteristic,
makeGetCharacteristicFeaturedImage,
} from '../variants';
import { wrapMemoizedSelector } from '../helpers';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry... the wrapMemoizedSelector module was also added to a PR which is already merged. So i move my comment from the last review here.

Could you please add a more detailed description to this function? It seems to test basic functionality of reselect, since it's an expected behaviour, that a selector returns the same value for multiple calls with the same args and state.

I think, there is no way to run into the error situation, even if the function would be called with a common selector, which was created without the make pattern. The only way to test the selector factories is to call them twice and compare the returned selector instances.

@fkloes fkloes merged commit c2f2e90 into v6.7.0 Jul 12, 2019
@fkloes fkloes deleted the PWA-2115-product-picture-updates-on-color-select branch July 12, 2019 09:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants