PWA-1707: Cleanup whishlist for render empty #3228
Conversation
|
| @@ -27,7 +27,10 @@ const WishlistPage = props => { | |||
| defaultMessage: 'The wishlist is not currently available.' | |||
| }); | |||
| const wishlistElements = useMemo(() => { | |||
| if (wishlists.length === 0) return null; | |||
| if (wishlists.length === 0) { | |||
| return <Wishlist data={[]} />; | |||
There was a problem hiding this comment.
With all the destructuring happening in the component, was surprised to see it not complain about this array value. Instead of making the placeholder dependent on the developer passing some arbitrary special value, let's instead make <Wishlist /> with no data prop behave like a placeholder. That paired with adding defaultProps to the component should accomplish the same thing with a better API.
Optionally, it looks like I missed adding propTypes on that component. Would be nice to document if you are going to pass data, what is expected in that object to render correctly.
| @@ -19,6 +19,10 @@ | |||
| grid-auto-flow: column; | |||
| } | |||
|
|
|||
| .emptyListText { | |||
| text-align: center; | |||
There was a problem hiding this comment.
Minor UX observation; mocks appear to have a minimum height that isn't reflected here. Giving the grid container a min-height is preferable here, but a simple line-height here could accomplish the same thing. If you want to attempt min-height solution, you'll need to give the grid alignment instructions, since it appears to want to stretch. If this has been approved by UX, no big deal.
There was a problem hiding this comment.
Why not a padding: 2rem 0; ?
There was a problem hiding this comment.
My preference is to let the grid align the content, but this is fine 👍
m2-community-project
bot
added
Progress: changes requested
and removed
Progress: ready for review
labels
m2-community-project
bot
added
Progress: review
and removed
Progress: changes requested
labels
packages/venia-ui/lib/components/WishlistPage/__tests__/wishlist.spec.js
Outdated
Show resolved
Hide resolved
| @@ -19,6 +19,10 @@ | |||
| grid-auto-flow: column; | |||
| } | |||
|
|
|||
| .emptyListText { | |||
| text-align: center; | |||
There was a problem hiding this comment.
My preference is to let the grid align the content, but this is fine 👍
…st.spec.js Co-authored-by: Tommy Wiebell <twiebell@adobe.com>
QA
|
supernova-at
added
the
version: Minor
Description
Clean up single wishlist
Related Issue
Closes PWA-1707
Acceptance
Verification Stakeholders
Specification
Verification Steps
My Account > Favorites shows a "there are currently no items in this list" message
After items are added and then removed back down to zero items, the "there are currently no items in this list" message shows again
Test Plan
Navigate to My Account > Favorites with a single, empty wishlist
See that the page shows the "no items" message
Add an item to the wishlist
Remove the item from the wishlist
See that the page again shows the "no items" message
Screenshots / Screen Captures (if appropriate)
Breaking Changes (if any)
Checklist