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.

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

Closed
wants to merge 1 commit into from

Conversation

lanadz-shopify
Copy link

@lanadz-shopify lanadz-shopify commented Feb 27, 2024

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".
image

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 title

Testing steps/scenarios

  • open https://quickstart-871572b0.myshopify.com/
  • add Snowboard bundle to the cart
  • Snowboard Bundle - With Title Override should be shown in the notification popup
  • Snowboard Bundle - With Title Override should be seen in the cart
  • Snowboard Bundle - With Title Override should be seen in the checkout
  • Repeat steps for (Magic carpet)[https://quickstart-871572b0.myshopify.com/products/magic-carpet] with update title checked
  • You can try any other products that don't receive cart transform overrides to verify that expected title is returned (product's title by default)

Demo links

Checklist

  • Added PR summary for release notes
  • Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
  • Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
  • Followed theme code principles
  • Linted with Theme Check
  • Tested on mobile There is no changes to mark up that need to be re-tested
  • Tested on multiple browsers There is no changes to mark up that need to be re-tested
  • Tested for accessibility There is no changes to mark up that need to be re-tested

@lanadz-shopify
Copy link
Author

@stefanlegg @dpwolf could you help with release notes for theme?

Copy link

@jmchauta jmchauta left a 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?

Copy link
Contributor

@ludoboludo ludoboludo left a 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>
Copy link
Contributor

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:

screenshot

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 🤔

cc: @melissaperreault

Copy link
Author

@lanadz-shopify lanadz-shopify Feb 28, 2024

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?

Copy link
Contributor

@melissaperreault melissaperreault Feb 28, 2024

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!

Copy link
Author

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.

cc @dpwolf @ignacio-chiazzo

Copy link
Author

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?

@lanadz-shopify
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants