Show default price for placeholder content #3572
Conversation
| @@ -114,6 +117,7 @@ | |||
| <div id="price-{{ section.id }}" role="status" {{ block.shopify_attributes }}> | |||
| {%- render 'price', | |||
| product: product, | |||
| placeholder: placeholder, | |||
There was a problem hiding this comment.
Is there any benefit to updating the existing placeholder logic on line 113 to use the same placeholder value, for consistency?
There was a problem hiding this comment.
Done! Updated a couple instances to use the placeholder check.
We shall if that's the approach we take based on Arthur's feedback 👍
| @@ -567,6 +567,7 @@ | |||
| {%- else -%} | |||
| {%- liquid | |||
| assign ratio = 1 | |||
| assign placeholder = true | |||
There was a problem hiding this comment.
Is this card-product snippet specifically for placeholder content?
There was a problem hiding this comment.
Yeah the else is specifically for placeholder content 👍
The if statement running before is if card_product and card_product != empty
| </s> | ||
| </span> | ||
| {%- endunless -%} | ||
| {%- unless target == null and placeholder == null -%} |
There was a problem hiding this comment.
Is the default value of an undefined liquid prop null? Would this check catch if placeholder was set explicitly to false?
There was a problem hiding this comment.
It is null 👍
I just ran this, and my nil got auto saved to null 🤦 , but it shows that it evaluates to null 🙂
|
I was looking at options for this on my end as well, and after going in circles for a while I got back to "what are we trying to say?" So I tried this hide.price.for.unavailable.variants.mp4 |
| </s> | ||
| </span> | ||
| {%- endunless -%} | ||
| {%- unless target == null and placeholder == null -%} |
There was a problem hiding this comment.
One thing I actually noticed which is minimal. But curious to know how we want to go about it.
There is an issue where if you add a feat. collection to the product page, the placeholder product cards get a price that matches the currently selected variant.
So with the placeholder prop I added, we can actually check if it is true and assign null to the target to keep our default 19.99 price as expected.
So I might keep the placeholder approach for that reason 🤔
The other solution could be to keep your approach with product != null but change it so it's not referring to the actual product in the snippet props. Where right now it says in the comment at the top of the file:
- product: {Object} Product Liquid object (optional)
Cause when on a product template, it will always return the product object of the template as a fallback.
It should maybe be named instead product_resource so we can actually know whether that prop was passed when rendering the snippet or not 🤔
And the file also points to product. a few times instead of target.
Let me know what you think. cc: @Roi-Arthur @emma-boardman @lhoffbeck
There was a problem hiding this comment.
Yep I can see the theoretical improvement as it preserves the same example price 👍🏼
On the other hand, I think the important piece is to show any price (rather than none), and ultimately the example shouldn't stick around for too long. I also somewhat doubt that Merchants would go around changing the product to see how it affects the example.
And lastly, if we're going back to the initial point that this messes up the AI generated themes, I think that this is mostly relevant to the front page.
Overall I think I would go for the least amount of code/complexity but happy to go either way.
There was a problem hiding this comment.
Readability wise, I prefer with placeholder. Makes it easy to understand quickly what we're checking. But as long as the issue is fixed it's what matters. Ill let others chime in.
There was a problem hiding this comment.
Ooh nice catch! Agree that while merchants may not notice, it'd be better to have consistent placeholder prices.
And +1 for placeholder, it's slightly more verbose than the 1-liner but makes the code more explicit/self-documenting and easier to understand. Is there anywhere else in our code (other than the place you already fixed 😄) that has similar placeholder behavior that we could update?
There was a problem hiding this comment.
Agree that placeholder is the right call here. Especially to avoid potentially breaking more things we don't know about, the more verbose solution is a lot safer.
There was a problem hiding this comment.
Is there anywhere else in our code (other than the #3572 (comment) 😄) that has similar placeholder behavior that we could update?
Some of the placeholder content is also set at "higher" level sometimes. I do it mostly in the card-product snippet as that's where it made the most sense imo so I it's applied anywhere the product card is called (collage, feat. collection). It's also called in search, collection, etc but those don't show placeholder product cards.
The other place using price where there is placeholder content is the feat. product. I just updated one more spot where I noticed the logic could be using the new placeholder variable.
* Make sure a default price is shown when we're showing placeholder content * address review comments * update one more spot to use the new variable
PR Summary:
We recently made changes to output the price in certain conditions.
Though there was an impact that we didn't notice, the default price shown for placeholder content is gone.
In this PR we're adding an extra check to render the price anyway if it's a placeholder context.
Why are these changes introduced?
To make sure placeholder prices are shown. Right now it looks like this:
What approach did you take?
Added a placeholder property to the price snippet. If it's not added, it will be
null, and if it's there we will still render the price.Other considerations
Visual impact on existing themes
Show the default price again
Testing steps/scenarios
Demo links
Checklist