Add Product Element #14773
Add Product Element #14773
Conversation
… colours for the product cards. Add the Product Card image component. Add the correct sizing to the Picture component for the new Product Image.
# Conflicts: # dotcom-rendering/src/types/content.ts
|
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. |
Co-authored-by: Jamie B <[email protected]>
| _type: 'model.dotcomrendering.pageElements.ProductBlockElement'; | ||
| elementId: string; | ||
| brandName: string; | ||
| starRating: string; |
There was a problem hiding this comment.
Should this be of type StarRating (defined in this file) or, if that doesn't work, number?
There was a problem hiding this comment.
This is type string because it can have a value of N/A, which is the undefined dropdown value. You can see the value set here
| customAttributes: { name: string; value: string }[]; | ||
| content: FEElement[]; | ||
| h2Id?: string; | ||
| displayType: ProductDisplayType; |
There was a problem hiding this comment.
Small thing, but do we need the word "type" in this type?
| for (const cta of ctas) { | ||
| const priceMatch = cta.price.match(/[\d,.]+/); | ||
| if (priceMatch) { | ||
| const priceNumber = parseFloat(priceMatch[0].replace(/,/g, '')); |
There was a problem hiding this comment.
What happens when parseFloat returns NaN1?
For example, if the first CTA is parsed as NaN, then I believe lowestPrice will be set to NaN? From then on, all comparisons with NaN evaluate to false2, so the condition in the if will always evaluate to false, and the first CTA will be chosen as the lowest price, regardless of the values?
Footnotes
| lowestPrice: getLowestPrice(element.productCtas), | ||
| }); | ||
|
|
||
| const getLowestPrice = (ctas: ProductCta[]): string | undefined => { |
There was a problem hiding this comment.
Would it be useful to add a JSDoc comment to this function explaining how these regexes and replacements work?
| product.displayType === 'ProductCardOnly' || | ||
| product.displayType === 'InlineWithProductCard'; | ||
| return ( | ||
| <div> |
There was a problem hiding this comment.
Could this div be a fragment?
| decorators: [ | ||
| (Story) => ( | ||
| <SectionComponent | ||
| shouldCenter={true} | ||
| showSideBorders={true} | ||
| centralBorder={'full'} | ||
| css={css` | ||
| strong { | ||
| font-weight: bold; | ||
| } | ||
| `} | ||
| > | ||
| <ArticleContainer | ||
| format={{ | ||
| design: ArticleDesign.Review, | ||
| display: ArticleDisplay.Standard, | ||
| theme: Pillar.Lifestyle, | ||
| }} | ||
| > | ||
| <Story /> | ||
| </ArticleContainer> | ||
| </SectionComponent> | ||
| ), | ||
| ], |
There was a problem hiding this comment.
As mentioned in the ProductCard PR, we have a centreColumnDecorator which may be a bit simpler here?
| decorators: [ | |
| (Story) => ( | |
| <SectionComponent | |
| shouldCenter={true} | |
| showSideBorders={true} | |
| centralBorder={'full'} | |
| css={css` | |
| strong { | |
| font-weight: bold; | |
| } | |
| `} | |
| > | |
| <ArticleContainer | |
| format={{ | |
| design: ArticleDesign.Review, | |
| display: ArticleDisplay.Standard, | |
| theme: Pillar.Lifestyle, | |
| }} | |
| > | |
| <Story /> | |
| </ArticleContainer> | |
| </SectionComponent> | |
| ), | |
| ], | |
| decorators: [centreColumnDecorator], |
| const productImage: ProductImage = { | ||
| url: 'https://media.guimcode.co.uk/cb193848ed75d40103eceaf12b448de2330770dc/0_0_725_725/725.jpg', | ||
| caption: 'Filter-2 test image for live demo', | ||
| height: 1, | ||
| width: 1, | ||
| alt: 'Bosch Sky kettle', | ||
| credit: 'Photograph: Rachel Ogden/The Guardian', | ||
| displayCredit: false, | ||
| }; | ||
|
|
||
| const product: ProductBlockElement = { |
There was a problem hiding this comment.
We could consider moving some of this data out into fixtures/manual? That may perhaps make the stories a bit easier to read, and it's possible some of it could be reused, as it looks like there's similar data used in other stories files and in the tests?
Co-authored-by: Jamie B <[email protected]>
| shouldShowLeftColCard; | ||
| const subheadingHtml = parseHtml( | ||
| `<h2 id="${product.h2Id ?? product.elementId}">${ | ||
| product.primaryHeading ? `${product.primaryHeading}</br>` : '' |
There was a problem hiding this comment.
A minor detail, but <br> is a self-closing tag so should be <br /> rather than </br>
Co-authored-by: Jamie B <[email protected]>
Co-authored-by: Charley_Campbell [email protected]
Co-authored-by: Emma Imber [email protected]
What does this change?
ProductElementcomponent and corresponding storyrenderElementcodecontentWhy?
We have added a new Product element in Composer, which needs to be handled in the rendering layer. The point of this element is to provide a way of structuring product data in Filter articles. This structured data will unlock more design opportunities on Filter pages. Some more details are here if you're interested.
Screenshots
Screenshots are taken using our test article in CAPI CODE
Wide breakpoint
Mobile