-
Notifications
You must be signed in to change notification settings - Fork 30
Add product carousel #14968
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
base: main
Are you sure you want to change the base?
Add product carousel #14968
Conversation
dotcom-rendering/src/components/ScrollableProduct.importable.tsx
Outdated
Show resolved
Hide resolved
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
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.
Looks good to me just one clarification question! I like the refactoring of the scrollable carousel - have we tested the carousels on the Fronts to check they haven't changed/broken?
I have not tested this, I will put it on code before merging. I was assuming that because there are no changes in chromatic it is probably fine but will test in CODE also |
|
Could we squash the commits where possible please. It would be great if each commit describes an atomic change and hence makes the history easier to read. |
… subgrid, and link to the product text heading using the H2. The product element heading h2Id is added using the enhancers in dcr
…f the card using Visable slides on either mobile or tablet. Remove the css in the carousel that is front specific and put that behind the !isArticle boolean
…l carousel. There will be a future PR for carousels with 2 cards + stories
f20853d to
a064393
Compare
| kind={CarouselKind.FixedWidthSlides} | ||
| carouselLength={products.length} | ||
| fixedSlideWidth={fixedCardWidth} | ||
| gapSizes={{ row: 'none', column: 'large' }} |
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 guess none is a useful option to have, although it's maybe not required here as shouldStackCards is not set so the carousel will always be on a single row meaning you don't need to worry about the row gap.
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.
Yea this is also helpful for when we don't want this set. Here where we are using a subgrid we then don't have to override it.
| */ | ||
| const containerStyles = css` | ||
| position: relative; | ||
| const frontContainerStyles = css` |
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.
Probably outside the scope of this PR, but I wonder if it would make sense to create a separate fronts carousel component that imports the base carousel and applies the necessary fronts specific styles? (Moving them from here.) That would, hopefully, leave a simpler generic carousel that could be more easily reused elsewhere.
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.
See comment below - Interested what you both think
| * A component used in the carousel fronts containers (e.g. small/medium/feature) | ||
| */ | ||
| export const ScrollableCarousel = ({ | ||
| kind, |
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.
Rather than passing this as a prop, have you considered exposing separate Carousel components that wrap a base carousel? This keeps the base carousel relatively clean and simple, while extracting and isolating the fancier logic and styles into the kinds of carousels that need them.
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.
Yea I did consider doing this, it was quite hard to work out what a base was for some things. For the fronts carousel there is a 32px peeping card but for our designs we have just a fixed width of cards at different breakpoints, the API for one doesn't really work for the other. There isn't an obvious base between these two, you could override one with the other by choosing one as the base but that seems worse than having 2 kinds IMO. This maybe because we don't have a generic one in the DS. Happy to chat about it though if you disagree.
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.
Yeah, my question came from a position of ignorance, I don't know how easy it would be to achieve. My thinking was actually to define a generic carousel (the elusive "DS carousel" you mention), and then use this as the base for a fronts-specific carousel and a product-specific carousel. It would be easier to do this now that we have two different carousel designs to work from and identify what carousel features we feel ought to be generic
…t much more intuitive. And allows us to remove the prop isArticle
…lableProduct.importable.tsx
What does this change?
Changes to ScrollableCarousel
type FixedSlideWidth = { defaultWidth: number; widthFromBreakpoints: { breakpoint: Breakpoint; width: number }[];}Choosing if to add padding or not. On the fronts padding is needed but for our article designs we do not.
isArticle: booleanAdds support for
subgridalignment within the cards by adding a newScrollableCarousel.SubgridItemWhy?
Screenshots