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

Show placeholder or featured media on product details page #756

Merged
merged 19 commits into from
Jul 19, 2019

Conversation

alexbridge
Copy link
Contributor

Description

Show placeholder / featured media on product details page when product media is not available

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.

@alexbridge alexbridge added the bug Something isn't working label Jul 5, 2019
@alexbridge alexbridge self-assigned this Jul 5, 2019
SG-Noxoreos and others added 5 commits July 5, 2019 20:46
… to `baseProduct` avoid confusion and improve clarity about what data it actually gets. Also renamed the state update method in `MediaImage` component to even better describe what it actually does.
Copy link
Contributor

@SG-Noxoreos SG-Noxoreos left a comment

Choose a reason for hiding this comment

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

  • I didn't see any issues with the code directly. Some testing revealed buggy behavior. When changing characteristic selections forth and back, the image, which was shown before is blank, when selecting a variant that was selected previously. This seems to happen only on the first characteristic.
    EDIT: Seems like this issue is not directly related to the changes done here.
  • Also, switching to a placeholder when changing characteristics appears like a visual bug, because it tends to show the placeholder for less than a second, making the transition appear to stutter (looks like three short flickers in a row, when images are not available, yet: it removes the image, shows the placeholder, removes the placeholder and shows the new image).

@coveralls
Copy link

coveralls commented Jul 15, 2019

Pull Request Test Coverage Report for Build 2917

  • 43 of 113 (38.05%) changed or added relevant lines in 23 files are covered.
  • 38 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.07%) to 50.984%

Changes Missing Coverage Covered Lines Changed/Added Lines %
libraries/engage/product/components/Media/FeaturedMedia.jsx 0 1 0.0%
libraries/engage/product/components/Media/MediaPlaceholder.jsx 3 4 75.0%
themes/theme-gmd/pages/Product/components/Characteristics/index.jsx 0 1 0.0%
themes/theme-gmd/pages/ProductGallery/components/Content/components/MediaSlider/index.jsx 0 1 0.0%
themes/theme-ios11/pages/Product/components/Characteristics/index.jsx 0 1 0.0%
themes/theme-ios11/pages/ProductGallery/components/Content/components/MediaSlider/index.jsx 0 1 0.0%
libraries/common/components/ProductCharacteristics/index.jsx 0 2 0.0%
libraries/engage/product/components/Media/MediaImage.jsx 3 6 50.0%
themes/theme-gmd/pages/ProductGallery/components/Content/components/MediaSlider/connector.js 1 4 25.0%
themes/theme-ios11/pages/Product/components/Media/index.jsx 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
themes/theme-ios11/pages/Product/components/Media/index.jsx 1 0.0%
libraries/engage/product/components/MediaSlider/index.jsx 2 14.81%
libraries/common/components/ProductCharacteristics/index.jsx 2 0.0%
libraries/common/subscriptions/error.js 33 0.0%
Totals Coverage Status
Change from base Build 2905: 0.07%
Covered Lines: 10073
Relevant Lines: 18727

💛 - Coveralls

@fkloes fkloes self-assigned this Jul 17, 2019
Copy link
Contributor Author

@alexbridge alexbridge left a comment

Choose a reason for hiding this comment

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

Changes are needed

libraries/engage/product/selectors/media.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@alexbridge alexbridge left a comment

Choose a reason for hiding this comment

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

iOs FeatureImage height is ~30% of media slider

@alexbridge alexbridge merged commit a9a4183 into v6.7.0 Jul 19, 2019
@alexbridge alexbridge deleted the PWA-2121-pdp-image-670 branch July 19, 2019 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants