Skip to content
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.

Sign up for GitHub

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

Show default price for placeholder content #3572

Merged
merged 3 commits into from
Jul 25, 2024
+104 −93
Merged

Show default price for placeholder content #3572

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1e30a9c
Make sure a default price is shown when we're showing placeholder con…
ludoboludo Jul 24, 2024
473f7e3
address review comments
ludoboludo Jul 25, 2024
6d9bab6
update one more spot to use the new variable
ludoboludo Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions sections/featured-product.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
{%- liquid
assign product = section.settings.product
if product == null
assign placeholder = true
endif
-%}

<product-info
Expand Down Expand Up @@ -114,6 +117,7 @@
<div id="price-{{ section.id }}" role="status" {{ block.shopify_attributes }}>
{%- render 'price',
product: product,
placeholder: placeholder,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any benefit to updating the existing placeholder logic on line 113 to use the same placeholder value, for consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Updated a couple instances to use the placeholder check.
We shall if that's the approach we take based on Arthur's feedback 👍

use_variant: true,
show_badges: true,
price_class: 'price--large'
Expand Down Expand Up @@ -372,11 +376,11 @@
render 'share-button', block: block, share_link: share_url
%}
{%- when 'variant_picker' -%}
{% render 'product-variant-picker',
product: product,
block: block,
product_form_id: product_form_id
%}

Check warning on line 383 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L379-L383 

[NestedSnippet] Too many nested snippets

Check warning on line 383 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L379-L383 

[NestedSnippet] Too many nested snippets

Check warning on line 383 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L379-L383 

[NestedSnippet] Too many nested snippets
{%- when 'buy_buttons' -%}
{%- render 'buy-buttons',
block: block,
Expand Down Expand Up @@ -465,7 +469,7 @@
if product.selected_or_first_available_variant.featured_media
assign seo_media = product.selected_or_first_available_variant.featured_media
else
assign seo_media = product.featured_media

Check warning on line 472 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L472 

[UnusedAssign] `seo_media` is never used
endif
-%}

Expand All @@ -479,7 +483,7 @@
{% endif %}
</product-info>

{% schema %}

Check notice on line 486 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L486 

[SchemaJsonFormat] JSON formatting could be improved
{
"name": "t:sections.featured-product.name",
"tag": "section",
Expand Down
3 changes: 2 additions & 1 deletion snippets/card-product.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,7 @@
{%- else -%}
{%- liquid
assign ratio = 1
assign placeholder = true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this card-product snippet specifically for placeholder content?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the else is specifically for placeholder content 👍
The if statement running before is if card_product and card_product != empty

if media_aspect_ratio == 'portrait'
assign ratio = 0.8
endif
Expand Down Expand Up @@ -610,7 +611,7 @@
<span class="visually-hidden">{{ 'accessibility.vendor' | t }}</span>
<div class="caption-with-letter-spacing light">{{ 'products.product.vendor' | t }}</div>
{%- endif -%}
{% render 'price', show_compare_at_price: true %}
{% render 'price', placeholder: placeholder, show_compare_at_price: true %}
</div>
</div>
</div>
Expand Down
172 changes: 88 additions & 84 deletions snippets/price.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

Accepts:
- product: {Object} Product Liquid object (optional)
- placeholder: {Boolean} Renders a placeholder price (optional)
- use_variant: {Boolean} Renders selected or first variant price instead of overall product pricing (optional)
- show_badges: {Boolean} Renders 'Sale' and 'Sold Out' tags if the product matches the condition (optional)
- price_class: {String} Adds a price class to the price element (optional)
Expand Down Expand Up @@ -37,94 +38,97 @@
endif
-%}

{%- unless target == nil -%}
<div
class="
price
{%- if price_class %} {{ price_class }}{% endif -%}
{%- if available == false %} price--sold-out{% endif -%}
{%- if compare_at_price > price and product.quantity_price_breaks_configured? != true %} price--on-sale{% endif -%}
{%- if compare_at_price > price and product.quantity_price_breaks_configured? %} volume-pricing--sale-badge{% endif -%}
{%- if product.price_varies == false and product.compare_at_price_varies %} price--no-compare{% endif -%}
{%- if show_badges %} price--show-badge{% endif -%}
"
>
<div class="price__container">
{%- comment -%}
Explanation of description list:
- div.price__regular: Displayed when there are no variants on sale
- div.price__sale: Displayed when a variant is a sale
{%- endcomment -%}
<div class="price__regular">
{%- if product.quantity_price_breaks_configured? -%}
{%- if show_compare_at_price and compare_at_price -%}
{%- unless product.price_varies == false and product.compare_at_price_varies %}
<span class="visually-hidden visually-hidden--inline">
{{- 'products.product.price.regular_price' | t -}}
</span>
<span>
<s class="price-item price-item--regular variant-item__old-price">
{% if settings.currency_code_enabled %}
{{ compare_at_price | money_with_currency }}
{% else %}
{{ compare_at_price | money }}
{% endif %}
</s>
</span>
{%- endunless -%}
{%- unless target == null and placeholder == null -%}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the default value of an undefined liquid prop null? Would this check catch if placeholder was set explicitly to false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is null 👍
I just ran this, and my nil got auto saved to null 🤦 , but it shows that it evaluates to null 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Recording

Let me know what you think. cc: @Roi-Arthur @emma-boardman @lhoffbeck

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<div
class="
price
{%- if price_class %} {{ price_class }}{% endif -%}
{%- if available == false %} price--sold-out{% endif -%}
{%- if compare_at_price > price and product.quantity_price_breaks_configured? != true %} price--on-sale{% endif -%}
{%- if compare_at_price > price and product.quantity_price_breaks_configured? %} volume-pricing--sale-badge{% endif -%}
{%- if product.price_varies == false and product.compare_at_price_varies %} price--no-compare{% endif -%}
{%- if show_badges %} price--show-badge{% endif -%}
"
>
<div class="price__container">
{%- comment -%}
Explanation of description list:
- div.price__regular: Displayed when there are no variants on sale
- div.price__sale: Displayed when a variant is a sale
{%- endcomment -%}
<div class="price__regular">
{%- if product.quantity_price_breaks_configured? -%}
{%- if show_compare_at_price and compare_at_price -%}
{%- unless product.price_varies == false and product.compare_at_price_varies %}
<span class="visually-hidden visually-hidden--inline">
{{- 'products.product.price.regular_price' | t -}}
</span>
<span>
<s class="price-item price-item--regular variant-item__old-price">
{% if settings.currency_code_enabled %}
{{ compare_at_price | money_with_currency }}
{% else %}
{{ compare_at_price | money }}
{% endif %}
</s>
</span>
{%- endunless -%}
{%- endif -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span class="price-item price-item--regular">
{{-
'products.product.volume_pricing.price_range'
| t: minimum: money_price_min, maximum: money_price_max
-}}
</span>
{%- else -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span class="price-item price-item--regular">
{{ money_price }}
</span>
{%- endif -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span class="price-item price-item--regular">
{{- 'products.product.volume_pricing.price_range' | t: minimum: money_price_min, maximum: money_price_max -}}
</span>
{%- else -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span class="price-item price-item--regular">
</div>
<div class="price__sale">
{%- unless product.price_varies == false and product.compare_at_price_varies %}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span>
<s class="price-item price-item--regular">
{% if settings.currency_code_enabled %}
{{ compare_at_price | money_with_currency }}
{% else %}
{{ compare_at_price | money }}
{% endif %}
</s>
</span>
{%- endunless -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.sale_price' | t }}</span>
<span class="price-item price-item--sale price-item--last">
{{ money_price }}
</span>
{%- endif -%}
</div>
<div class="price__sale">
{%- unless product.price_varies == false and product.compare_at_price_varies %}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.regular_price' | t }}</span>
<span>
<s class="price-item price-item--regular">
{% if settings.currency_code_enabled %}
{{ compare_at_price | money_with_currency }}
{% else %}
{{ compare_at_price | money }}
{% endif %}
</s>
</div>
<small class="unit-price caption{% if product.selected_or_first_available_variant.unit_price_measurement == nil %} hidden{% endif %}">
<span class="visually-hidden">{{ 'products.product.price.unit_price' | t }}</span>
<span class="price-item price-item--last">
<span>{{- product.selected_or_first_available_variant.unit_price | money -}}</span>
<span aria-hidden="true">/</span>
<span class="visually-hidden">&nbsp;{{ 'accessibility.unit_price_separator' | t }}&nbsp;</span>
<span>
{%- if product.selected_or_first_available_variant.unit_price_measurement.reference_value != 1 -%}
{{- product.selected_or_first_available_variant.unit_price_measurement.reference_value -}}
{%- endif -%}
{{ product.selected_or_first_available_variant.unit_price_measurement.reference_unit }}
</span>
</span>
{%- endunless -%}
<span class="visually-hidden visually-hidden--inline">{{ 'products.product.price.sale_price' | t }}</span>
<span class="price-item price-item--sale price-item--last">
{{ money_price }}
</span>
</small>
</div>
<small class="unit-price caption{% if product.selected_or_first_available_variant.unit_price_measurement == nil %} hidden{% endif %}">
<span class="visually-hidden">{{ 'products.product.price.unit_price' | t }}</span>
<span class="price-item price-item--last">
<span>{{- product.selected_or_first_available_variant.unit_price | money -}}</span>
<span aria-hidden="true">/</span>
<span class="visually-hidden">&nbsp;{{ 'accessibility.unit_price_separator' | t }}&nbsp;</span>
<span>
{%- if product.selected_or_first_available_variant.unit_price_measurement.reference_value != 1 -%}
{{- product.selected_or_first_available_variant.unit_price_measurement.reference_value -}}
{%- endif -%}
{{ product.selected_or_first_available_variant.unit_price_measurement.reference_unit }}
</span>
{%- if show_badges -%}
<span class="badge price__badge-sale color-{{ settings.sale_badge_color_scheme }}">
{{ 'products.product.on_sale' | t }}
</span>
</small>
</div>
{%- if show_badges -%}
<span class="badge price__badge-sale color-{{ settings.sale_badge_color_scheme }}">
{{ 'products.product.on_sale' | t }}
</span>

<span class="badge price__badge-sold-out color-{{ settings.sold_out_badge_color_scheme }}">
{{ 'products.product.sold_out' | t }}
</span>
{%- endif -%}
</div>
{% endunless %}
<span class="badge price__badge-sold-out color-{{ settings.sold_out_badge_color_scheme }}">
{{ 'products.product.sold_out' | t }}
</span>
{%- endif -%}
</div>
{% endunless %}
Loading