Conversation
🦋 Changeset detectedLatest commit: 6c10aad The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
jorgemoya
left a comment
jorgemoya
left a comment
There was a problem hiding this comment.
Looking good, just have a couple of comments!
| const baseVariant = removeEdgesAndNodes(product.variants).at(0); | ||
|
|
||
| if (!baseVariant?.inventory?.byLocation) { | ||
| return undefined; | ||
| } | ||
|
|
||
| const inventoryByLocation = removeEdgesAndNodes(baseVariant.inventory.byLocation).at(0); | ||
|
|
||
| return inventoryByLocation?.backorderMessage || undefined; |
There was a problem hiding this comment.
Can you add a comment of what this is doing? I'm not familiar why this !baseVariant?.inventory?.byLocation warrants no message.
Should we also default a backorderMessage that we define in the Catalyst dictionary instead of returning undefined?
There was a problem hiding this comment.
It's up to the merchant to decide whether to display a backorder message or not by allocating a backorder message to a product / product variant. So, it can be undefined.
jorgemoya
left a comment
jorgemoya
left a comment
There was a problem hiding this comment.
The problem I'm seeing with this feature is using strings defined by the merchant in the CP that are not multi-lang or translatable (as far as we know). We've skipped using these fields in Catalyst so that the translated experience remains consistent. To me, a good workaround is to simply use the Catalyst dictionary instead of fetching from GQL.
Tagging our Catalyst PM to provide some input on this. @bc-erich
| fragment ProductVariantsInventoryFragment on Product { | ||
| variants { | ||
| edges { | ||
| node { | ||
| entityId | ||
| sku | ||
| inventory { | ||
| byLocation { | ||
| edges { | ||
| node { | ||
| locationEntityId | ||
| backorderMessage | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| `); |
There was a problem hiding this comment.
Seems to me you only use the base variant. Could you modify this fragment to only include the first (unless I'm missing something)?
There was a problem hiding this comment.
The same fragment will be used in product details page as well. So, I wanted to avoid duplicate code.
There was a problem hiding this comment.
This is a very taxing query that is multiplied by number of products in a given PLP page. For PDP it's not a big deal, but if you are showing 20-30 products in a given PLP, I believe this is simply too much.
There's two options:
- Duplicate the code
- Fragment can include a
firstprop (examplePricingFragment)
There was a problem hiding this comment.
I'll go with option one
| <span | ||
| className={clsx( | ||
| 'block font-semibold', | ||
| 'line-clamp-2 font-semibold', |
There was a problem hiding this comment.
Can we include changes like these in a separate PR for review?
There was a problem hiding this comment.
Why can't we get it reviewed in this PR? 🤔
There was a problem hiding this comment.
This is a complex PR that will be difficult for consumers of Catalyst to merge into their custom codebases. If this change is not related to the intent of this PR, it is much easier for consumers to see this change as a separate PR that is tracked with it's own migration and changeset release notes.
There was a problem hiding this comment.
This change to limit the product name to product name on the card to two line followed by "..." if longer, is requested by the Jess as per her agreement with @andrewreifman. It's related to this PR to control the card size/layout while adding more info to it which is the backorder message.
| numberOfReviews | ||
| averageRating | ||
| } | ||
| variants { |
There was a problem hiding this comment.
Still need to query only the first one, since it seems that is all you need.
| variants { | |
| variants(first: 1) { |
| - Add the backorder message if the product has no on-hand stock and is available for backorder and the store/product is set to display the backorder message | ||
|
|
||
| ## Migration | ||
| For existing Catalyst stores, to get the newly added feature, simply rebase the existing code with the new release code. The files to be rebased for this change to be applied are: |
There was a problem hiding this comment.
Can you leverage AI and see if it can give you a better migration path for merchants that don't rebase?
There was a problem hiding this comment.
AI did not do a good job extracting the detailed steps for migration. So, I explained it myself in more details.
3 participants
What/Why?
Display the backorder message or out-of-stock messages on the product card in different product lists according to the product stock status and the store settings.
Testing
Screenshots
Home page featured products lists
Category PLP
Brand PLP
Searc
h PLP
Migration
While rebasing, there might be some conflicts if there is some custom styling of the product card component. Those conflicts may occur in the following files: