-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Cart transform can send the title override for cart #3304
Conversation
@stefanlegg @dpwolf could you help with release notes for theme? |
31d7729
to
ca5dadd
Compare
ca5dadd
to
d323825
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.
Code LGTM, does the 🎩 cover the snippets/cart-drawer.liquid
surface?
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.
Flagging this to get alignment from a theme designer as I'd see this as a visual regression
@@ -16,7 +16,7 @@ | |||
{%- if settings.show_vendor -%} | |||
<p class="caption-with-letter-spacing light">{{ item.product.vendor }}</p> | |||
{%- endif -%} | |||
<h3 class="cart-notification-product__name h4">{{ item.product.title | escape }}</h3> | |||
<h3 class="cart-notification-product__name h4">{{ item.title | escape }}</h3> |
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.
I think this create a bit of an ugly fallback that isn't wanted.
Here is a comparison of the before vs after:
We're already mentioning the variant/options picked in a nicer visual way and this is repeating that content.
We should involve a theme UX designer to get their opinion on what would be best here.
It doesn't seem like we can tell when a transform has been applied or not right ? There isn't logic we could apply to it to only use item.title
when a transform is detected 🤔
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.
We're already mentioning the variant/options picked in a nicer visual way and this is repeating that content.
Thanks for feedback! Yeah, agree that this might be not the nicest representation.
There isn't logic we could apply to it to only use item.title when a transform is detected 🤔
We might expand and add such logic to liquid. Or introduce new entire property for cart item, for instance item.presentment_title
Another option would be can redefine item.title
to be the same as item.product.title
instead of concatenation of product title + variant title
From representing point of view, what is the purpose of cart.item.title
? would it make sense to be the same as product title for dawn or other themes?
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.
+1 to @ludoboludo's concerns: repeating the variant name in the product title is redundant for all buyers, including buyers using voice over. I am sure there are more clever outcome we can explore 🙏
For example, when selling plans are available, it would be listed as a line item under the variant choices details; both in cart and checkout.
Curious to learn where this lands!
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.
Thank you @melissaperreault and @ludoboludo!
Example with selling plans makes much sense, we probably should look into some conditional rendering here as well.
contextually for existing representation it makes sense to keep item.product.title
and with listed variant details under. For all merchants that not using cart transform it is not needed regression. Overriding product.title
doesn't seem right either, as we are not overriding product's title, but cart item's title.
I will need to speak with product about this one, but suspect the best solution would be probably to introduce a new field here to have something like item.title_override?
and item.title_override
for this specific use case.
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.
@dpwolf I think we might need some UX advice here,
would it make sense to have some kind of different behaviour in case if we have title override?
have something like this in default case (current behaviour)
- product title
- variant title 1
- variant title 2
and totally different rendering for case when we have title overrides, like skipping the variants values and have only
title override for line instead of product title - variant title 1/variant title 2
Should it even be on theme by default? or those changes have to be available for customization for each merchant for them to decide?
I am closing this pr, as there is undesirable regression and we need to come up with better solution how to show cart line title override. |
PR Summary:
Cart Transform feature allows partners to override the title in the cart and checkout. For example, if "Fancy Watch" and "Warranty" are added to the cart, bundle is created and renamed as "Fancy Watch with warranty".
More details about cart transform can be found here
Why are these changes introduced?
Fixes [#67257 Display override title in cart line]https://github.com/Shopify/core-issues/issues/67257
Feature is released as of 2024-01, sfr is one of the surfaces that are not showing updated title for cart
What approach did you take?
Other considerations
Titles changed by cart transform operations will not be translated
Visual impact on existing themes
Merchants who are using cart transform feature -they will see title overrides in cart.
This will have no impact to merchants who are not using Cart Transform, as
item.title
falls back to variant's title , which is defined here and eventually goes to product titleTesting steps/scenarios
Snowboard Bundle - With Title Override
should be shown in the notification popupSnowboard Bundle - With Title Override
should be seen in the cartSnowboard Bundle - With Title Override
should be seen in the checkoutupdate title
checkedDemo links
Checklist