-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat: add product card [MONET-1338] #4534
Conversation
b65800b
to
bd09390
Compare
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.
Just some minor comments after reviewing the code! Looking great. For some odd reason when I spin up Storybook locally, it is only showing a handful of the components that you added (missing ProductCard
etc). This could absolutely be a 'me' issue but wondering if I am missing a step (pulling down branch, install, and npm run storybook
).
...mmerce-app-base/src/ProductCard/ProductCardAdditionalData/ProductCardAdditionalDala.spec.tsx
Outdated
Show resolved
Hide resolved
packages/ecommerce-app-base/src/ProductCard/ProductCardBody/ProductCardBody.tsx
Outdated
Show resolved
Hide resolved
17e2655
to
1535b7e
Compare
1535b7e
to
e6d8a4d
Compare
Thanks for your review 🙏 |
✅ Deploy Preview for ecommerce-app-base-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
f9b1512
to
828b5d0
Compare
e76a947
to
63951cf
Compare
63951cf
to
d279ea0
Compare
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.
This is looking great! Was able to look at everything in Storybook locally. Thank you for setting that up, as it makes for such easy visual and functional validation. I had a few minor comments and one more general question:
- What is the outlook on deprecating the v1 of the
productCard
? Are we looking to measure the success of v2 somehow and then deprecate the old one eventually? Just want to make sure we document this process if this falls to us to address eventually! - Will there be documentation introduced within this package or the README to guide the user of the package for how to use v2 of the components?
.../ecommerce-app-base/src/AdditionalDataRenderer/MetaDataRenderer/MetaDataRenderer.stories.tsx
Outdated
Show resolved
Hide resolved
packages/ecommerce-app-base/src/AdditionalDataRenderer/RawDataRenderer/RawDataRenderer.tsx
Outdated
Show resolved
Hide resolved
fireEvent(image, new Event('error')); | ||
expect(image).not.toBeInTheDocument(); | ||
expect(getByTestId('asset-icon')).toBeInTheDocument(); | ||
}); | ||
|
||
it('should render successfully the error variation for missing product', async () => { | ||
// | ||
it.skip('should render successfully the error variation for missing product', async () => { |
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.
is there a specific reason that this test is skipped?
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.
Great stuff, some minor comments+questions.
products: Product[]; | ||
}; | ||
|
||
export type ProductsFn = ( |
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.
export type ProductsFn = ( | |
export type FetchProductsFn = ( |
hasNextPage?: boolean; | ||
}; | ||
|
||
type ProductsFnResponse = { |
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.
Does it make sense to make this generic like this:
type FetchFnResponse<I> = {
pagination: Pagination
items: I[]
}
packages/ecommerce-app-base/src/SkuPicker/ProductList/ProductList.tsx
Outdated
Show resolved
Hide resolved
packages/ecommerce-app-base/src/ProductCard/ProductCardMenu/ProductCardMenu.tsx
Show resolved
Hide resolved
describe('A RawDataRenderer', () => { | ||
it('provides a copy button', () => { | ||
const { getByRole } = render(<RawDataRenderer value={VALUE} />); | ||
const button = getByRole('button'); |
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.
To make this more expressive this should include the a11y label besides the role.
packages/ecommerce-app-base/src/AdditionalDataRenderer/RawDataRenderer/RawDataRenderer.tsx
Show resolved
Hide resolved
products: P[]; | ||
}; | ||
|
||
export type ProductsFn<P extends Product = Product> = ( |
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.
export type ProductsFn<P extends Product = Product> = ( | |
export type FetchProductsFn<P extends Product = Product> = ( |
type ProductsFnResponse<P extends Product = Product> = { | ||
pagination: Pagination; | ||
products: P[]; | ||
}; |
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.
type ProductsFnResponse<P extends Product = Product> = { | |
pagination: Pagination; | |
products: P[]; | |
}; | |
type FetchFnResponse<Item = unknown> = { | |
pagination: Pagination; | |
items: Item[]; | |
}; |
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 really great Marco! Thanks for putting so much thought and detail into this work, it really shows ✨ . Approving because code and local storybook looks awesome, but still curious about the following next-steps:
What is the outlook on deprecating the v1 of the productCard? Are we looking to measure the success of v2 somehow and then deprecate the old one eventually? Just want to make sure we document this process if this falls to us to address eventually!
Will there be documentation introduced within this package or the README to guide the user of the package for how to use v2 of the components?
Thanks in advance for final clarifications!
# Conflicts: # packages/ecommerce-app-base/package-lock.json
@lilbitner To have at least some visibility of the API changes, I have re-generated the local docs, and added a link to the storybook instance to the readme. For all other questions, we can use the doc I shared earlier with you :) |
Purpose
Introduce a new design for the product card.
Approach
To not enforce this change from the start, we allow consumers to opt-in to the new version of the ProductCard.
Alternatively, we could release with a major version bump.
As foundation for the new ProductCard, the same component has been copied over from the ecommerce app, and changed to our needs.
We will also allow to render additional data (provider specific) from the consumer directly, by implementing an
additionalDataRenderer
Function on the Integration object.We ship with two predefined renderer for additional data:
RawDataRenderer
renders any object as JSON stringMetaDataRenderer
renders a grey box with columns and data itemsTesting steps
Since there is currently no consumer for this, but we build a new storybook instance on every build, a link can be found in the PR comments.
How it works: