-
Notifications
You must be signed in to change notification settings - Fork 145
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
[Shopper Experience] ImageTile
component
#967
Conversation
ratio: PropTypes.number, | ||
caption: PropTypes.string |
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.
Where did these props come from? Below is what the API returns for a ImageTile
component:
"data": {
"image": {
"_type": "Image",
"focalPoint": {
"_type": "Imagefocalpoint",
"x": 0.5,
"y": 0.5
},
"metaData": {
"_type": "Imagemetadata",
"height": 179,
"width": 410
},
"url": "https://development-functional38-qa222.demandware.net/on/demandware.static/-/Library-Sites-RefArchSharedLibrary/default/dwd35a6963/placeholder 410x179.4.png"
}
},
So ideally we implement those props and nothing else.
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.
Currently my implementation of the base components will spread the serialized component onto the provided component.
So technically the only prop you'd have now is id
, data
and typeId
. I'm wondering if this is something we should change to make the component implementations look nicer and more reusable.
Lets talk about that tomorrow.
<source srcSet={imageProps.src?.tablet} media="(min-width: 48em)" /> | ||
<source srcSet={imageProps.src?.desktop} media="(min-width: 64em)" /> | ||
<Image | ||
className={'photo-tile-image image-fluid'} |
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.
Lets drop any utility classes if they aren't doing anything. Our convention currently in the template is to use inline styles (I think thats our convention).
Also, I'm guessing the reason why the class name for the sfra component is called photo tile, is because that was it's initial implementation and then it was changed on the surface because someone didn't like the look of it in the page designer.
So if we choose to use classnames for our components we should use the same name as our files. So in this case image-tile
not photo-tile
ignoreFallback={true} | ||
alt={image?.alt} | ||
title={image?.alt} | ||
{...rest} |
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.
Do we need this "rest" object? For the most part this component isn't going to be used directly by a developer, it'll only be created by the Page/Region components, so we don't need this ability to passthrough props. Especially when it makes defining your PropTypes impossible.
*/ | ||
const ImageTile = ({image, ...rest}) => { | ||
return ( | ||
<figure className={'image-tile-figure'}> |
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.
From the looks of the components in SFRA they have a pattern where the outermost element of the component is labelled as "-container". Although I'm not saying we need to duplicate this 1-1 I do think that the outermost element should be something other than a "figure" element.
A div with a class name of image-tile
would probably suffice.
* develop: (52 commits) fix localstorage key by add siteId prefix (#988) [Shopper Experience] `ImageTile` component (#967) Skip Snyk CLI (#983) fix(retail-react-app): recent searches (#969) [Feature] Shopper Experience / Page Designer Components (#963) fix(template-retail-react-app): improved handling of blocked einstein requests (#970) remove getResetPasswordToken from not implemented list (#973) Apply changes from commerce react sdk in feature branch (#964) Add Shopper Experience hooks (#958) chore: update pwa-kit-dev eslint config (#950) Serialize category data once only (#953) Fix layout shift for mega menu (#952) Update customer baskets cache when there is a basket mutation (#945) Remove the PersistentCache functionality (#949) Replace isomorphic jest mocks with msw handlers (#944) fix: handle special characters in `boldString` util (#942) Fix broken CI – confusing path to package.json in the 'dist' directory! (#946) @W-11920099 Re-write 'npm push' in Typescript, warn if Node deprecated (#763) Allow support for multiple sites concurrently w/ `commerce-sdk-react` (#911) Update `develop` with v2.6.0 (#939) ... # Conflicts: # lerna.json # package-lock.json # package.json # packages/commerce-sdk-react/CHANGELOG.md # packages/commerce-sdk-react/package-lock.json # packages/commerce-sdk-react/package.json # packages/internal-lib-build/package-lock.json # packages/internal-lib-build/package.json # packages/pwa-kit-create-app/package-lock.json # packages/pwa-kit-create-app/package.json # packages/pwa-kit-dev/CHANGELOG.md # packages/pwa-kit-dev/bin/pwa-kit-dev.js # packages/pwa-kit-dev/package-lock.json # packages/pwa-kit-dev/package.json # packages/pwa-kit-react-sdk/CHANGELOG.md # packages/pwa-kit-react-sdk/package-lock.json # packages/pwa-kit-react-sdk/package.json # packages/pwa-kit-react-sdk/src/ssr/browser/main.jsx # packages/pwa-kit-runtime/CHANGELOG.md # packages/pwa-kit-runtime/package-lock.json # packages/pwa-kit-runtime/package.json # packages/template-express-minimal/package-lock.json # packages/template-express-minimal/package.json # packages/template-retail-react-app/app/commerce-api/mock-data.js # packages/template-retail-react-app/package-lock.json # packages/template-retail-react-app/package.json # packages/template-typescript-minimal/package-lock.json # packages/template-typescript-minimal/package.json # packages/test-commerce-sdk-react/package-lock.json # packages/test-commerce-sdk-react/package.json
Description
The PR adds the equivalent SFRA Page designer
ImageTile
component to the PWA.Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization