Feature: Page Designer Carousel Component #977
Conversation
bendvc
added
ready for review
| // Our indicator implementation uses the scrollbar to show the context of the current | ||
| // item selected. Because MacOS hides scroll bars after they come to rest we need to | ||
| // force them to show. Please note that this feature only works on web-kit browsers, | ||
| // for all other brosers the scroller/indicator will be shown. | ||
| const css = ` | ||
| .scroll-indicator::-webkit-scrollbar { | ||
| display:${overflowXScrollValue}; | ||
| -webkit-appearance: none; | ||
| height: 8px; | ||
| } | ||
| .scroll-indicator::-webkit-scrollbar-thumb { | ||
| background-color: rgba(0, 0, 0, 0.5); | ||
| } |
There was a problem hiding this comment.
The SFRA carousel shows one indicator per element, highlighting the current element. Are we missing implementing that navigation?
There was a problem hiding this comment.
As discussed IRL our design here is diverging as we have decided to base out implementation off of a previous already implemented component called the product slider. This was done to save implementation time, 1-1 parity with the UI from SFRA and PWA kit is the tradeoff. However this design, because it's based off of a pre-existing component, fits the UI better.
| // Scroll the container left or right by 100%. Passing no args or `1` | ||
| // scrolls to the right, and passing `-1` scrolls left. | ||
| const scroll = useCallback((direction = 1) => { | ||
| scrollRef.current?.scrollBy({ | ||
| top: 0, | ||
| left: (direction * window.innerWidth) / itemCount, | ||
| behavior: 'smooth' | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The scrolling in the SFRA carousel by default implements an infinite loop behaviour when scrolling in either direction. Is this something we need to implement?
There was a problem hiding this comment.
When this PR was first estimated, it was estimated with the idea that we were going to reuse the code from the product scroller. Because this implementation uses a simple overflow with snap, it would be significant effort to make the carousel behave exactly like sfra. I think it's ok to not verbatim match srfa's implementation, instead be more inline with the PWA implementation
| {/* Button Controls */} | ||
| <Fragment> | ||
| <Box | ||
| display={controlDisplay} |
There was a problem hiding this comment.
The SFRA carousel only shows the control buttons when the items don't fit in the current view.
i.e. left/right scroll buttons are not rendered when the elements are less or equal to the defined number of CarouselSlidesToDisplay that fit in the view.
There was a problem hiding this comment.
I think this is valuable. I've added logic to detect if there is overflow and only if there is overflow will it obey the control visibility prop configurations.
| style={{scrollSnapAlign: 'start'}} | ||
| > | ||
| <AspectRatio ratio={1}> | ||
| <Component component={{...component, index}} /> |
There was a problem hiding this comment.
The prop index is not a key that exists in the component API response. We may want to remove it before merging.
There was a problem hiding this comment.
Good call. I did this because I was passing that index through and using it for the image provider as an id value. I've changed the usage for my example to no require the id value, instead it uses the components id as a seed to get the image. Not that it's important because that page is only an example and isn't going to be commited.
|
|
||
| return ( | ||
| <Box className={'carousel'} position="relative" data-testid="carousel"> | ||
| <style>{css}</style> |
There was a problem hiding this comment.
Is this the proper way to add custom CSS? Do we want to use the sx or __css Chakra props?
The browser console is showing the warning:
Warning: Did not expect server HTML to contain the text node
Probably unrelated to this CSS usage tho.
There was a problem hiding this comment.
Been soo long working on component that I just forgot. I've changed it to use the sx prop.
| import React, {Fragment, useCallback, useRef} from 'react' | ||
| import PropTypes from 'prop-types' | ||
| import {AspectRatio, Box, Heading, IconButton, Stack, useBreakpointValue} from '@chakra-ui/react' | ||
| import {Component} from 'commerce-sdk-react-preview/components' |
There was a problem hiding this comment.
A reminder that I understood we mentioned that we want to import these components directly in the template for now until we release the new commerce-sdk package.
There was a problem hiding this comment.
From the sounds of it, we'll be copying over those components from the library into the template, we'll have to update this import.
This isn't relevant for this PR, but will be later.
| 'img-src': [ | ||
| "'self'", | ||
| '*.commercecloud.salesforce.com', | ||
| 'picsum.photos', | ||
| '*.picsum.photos', | ||
| 'data:' | ||
| ], |
There was a problem hiding this comment.
TODO: Remove this before merging the PR
packages/template-retail-react-app/app/components/experience/layouts/carousel/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/experience/layouts/carousel/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/experience/layouts/carousel/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/experience/layouts/carousel/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/experience/layouts/carousel/index.jsx
Outdated
Show resolved
Hide resolved
…ayouts/carousel/index.jsx Co-authored-by: Adam Raya <[email protected]>
…ayouts/carousel/index.jsx Co-authored-by: Adam Raya <[email protected]>
…ayouts/carousel/index.jsx Co-authored-by: Adam Raya <[email protected]>
…ayouts/carousel/index.jsx Co-authored-by: Adam Raya <[email protected]>
…ayouts/carousel/index.jsx Co-authored-by: Adam Raya <[email protected]>
…b.com/SalesforceCommerceCloud/pwa-kit into feature/experience-carousel-component
* develop: Support Node 16 (#965) [W-12450361] Introduce short-circuit method for bypassing auth in Commerce Provider by passing in a fetchedToken (#1010) remove jest-silent-reporter (#1009) Clean up Page Designer Code (#1004) [Shopper Experience] `ImageWithText` component (#991) Feature: Page Designer Carousel Component (#977) [Feature] Page Designer Layout Components WIP (#993) remove updatePw from not implemented list (#996) Back-port Shopper Experience Base Components into Retail Template (#992) # Conflicts: # packages/commerce-sdk-react/package-lock.json # packages/commerce-sdk-react/package.json # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-dev/src/configs/webpack/config.js # packages/template-retail-react-app/package-lock.json
Description
This PR implements the Page Designer Carousel component. The implementation takes its design from the preexisting Product Scroller component in order to cut down on implementation time due to its simple implementation. The component achieves its carousel like feel using the
scrollSnapTypeproperty, as a result the scroll indicator / pip's that are in the SFRA implementation manifest themself as the horizontal scroll bar. The visibility of this indicator/scrollbar is controlled by themdCarouselIndicatorsoption set in page designer. One caveat however is that the indicator is always shown on non-webkit browsers for the time being.Types of Changes
Changes
How to Test-Drive This PR
git checkout feature/experience-carousel-componentnpm cinpm starthttp://localhost:3000/page-viewersample-page.jsonfile. Inside you'll find a serialized page, you can edit the options for the carousel by searching forcommerce_layouts.carouseland editing thedataobject values for that object. E.g. change the number of items to show for the given device width or whether or not the controls and/or indicators are showing.Checklists
General
TODO