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

[Gift card] Add help doc link and change label for translation #2471

Merged
merged 9 commits into from
Apr 4, 2023

Conversation

ludoboludo
Copy link
Contributor

PR Summary:

This is to add a link to the help docs in the editor. So that if the merchant needs more info about how to add a gift card product, it's easy to get to.
It also changes a label so that if they're editing their language file, the label is a bit clearer.

Why are these changes introduced?

Solves a couple issues mentioned in #2454

What approach did you take?

Linked to the help doc explaining how to add a gift card product.

Other considerations

Decision log

# Decision Alternatives Rationale Downsides
1

Visual impact on existing themes

No visual impact on the theme. Just some added context to the editor.

Testing steps/scenarios

  • Make sure everything about gift card works as expected. Go to a product page, edit the buy buttons block and enable the gift card feature.
  • Go to a gift card product and make sure everything is working. When clicking the checkbox it will show a form.

Demo links

Checklist

@@ -866,7 +866,7 @@
},
"show_gift_card_recipient": {
"label": "Show recipient form for gift card products",
"info": "When enabled, gift card products can optionally be sent to a recipient with a personal message."
"info": "When enabled, gift card products can optionally be sent to a recipient with a personal message.[Learn more](https://help.shopify.com/en/manual/products/gift-card-products/add-update-gift-card-products)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the featured product section but the setting isn't present on that section. So not sure if we should remove it or leave it there for when it's added in the future 🤔 cc: @antiBaconMachine

Copy link
Contributor

@kmeleta kmeleta Mar 29, 2023

Choose a reason for hiding this comment

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

If we know for sure it's getting added I feel like it's probably fine to leave in, but I would also say I wouldn't mind just repurposing the main-product entries in featured-product instead of duplicating them here. We've started doing that more.

@eugenekasimov eugenekasimov self-requested a review March 27, 2023 20:26
@@ -459,7 +459,7 @@
"form": {
"checkbox": "I want to send this as a gift",
"email_label": "Recipient email",
"email_label_optional": "Recipient email (optional)",
"email_label_optional_for_no_js_behavior": "Recipient email (optional)",
Copy link
Contributor

Choose a reason for hiding this comment

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

Little wordy no?

Suggested change
"email_label_optional_for_no_js_behavior": "Recipient email (optional)",
"email_label_no_js": "Recipient email (optional)",

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 but that's what is turned into the wording in the admin 😬
So I wasn't sure if Email label no js was clear enough for non technical folks. Not that Email label optional for no js behaviour is much clearer but if you're looking for that means on the internet, it seems maybe easier 🤷

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably rather it be more concise, but it's not a hill I'll die on by any stretch, so if you think this is better that's fine with me.

Honestly, I'm not even sure it would be wrong to just use email_label_optional either. That's probably more inline with standard procedure, but I could see that being perceived as a mistake too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it's fine to keep it wordy in json files because I think it's better to be more descriptive there rather than compact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

email_label_optional is confusing/vague for a merchant I think.
Here is the current look of it:

Screenshot

There are more opportunities to make it clearer. Name and Message can be confusing as well. They're used as placeholders which is only picked up when using a screen reader:

Screenshot

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I should have followed this back to the line item in the issue. I understand where you're coming from now. I'm fine with what you had originally 👍

@@ -866,7 +866,7 @@
},
"show_gift_card_recipient": {
"label": "Show recipient form for gift card products",
"info": "When enabled, gift card products can optionally be sent to a recipient with a personal message."
"info": "When enabled, gift card products can optionally be sent to a recipient with a personal message.[Learn more](https://help.shopify.com/en/manual/products/gift-card-products/add-update-gift-card-products)"
Copy link
Contributor

@kmeleta kmeleta Mar 29, 2023

Choose a reason for hiding this comment

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

If we know for sure it's getting added I feel like it's probably fine to leave in, but I would also say I wouldn't mind just repurposing the main-product entries in featured-product instead of duplicating them here. We've started doing that more.

Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

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

Looks good. Let's wait for translations.

@@ -1502,6 +1502,7 @@ a.product__text {
align-items: flex-start;
max-width: inherit;
position: relative;
cursor: pointer;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this

Comment on lines -867 to -869
"show_gift_card_recipient": {
"label": "Show recipient form for gift card products",
"info": "When enabled, gift card products can optionally be sent to a recipient with a personal message."
Copy link
Contributor

Choose a reason for hiding this comment

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

Noticed this wording was actually a little different from the main product entry anyway (and it probably shouldn't have been).

@ludoboludo
Copy link
Contributor Author

FYI, I just had to replace the actual link I was using. A new doc was shipped by the gift card team and the link I had before was passing in the language when it shouldn't:
before

https://help.shopify.com/en/manual/products/gift-card-products/add-update-gift-card-products)

after

https://help.shopify.com/manual/online-store/themes/customizing-themes/add-gift-card-recipient-fields

@ludoboludo ludoboludo merged commit b8bfb79 into main Apr 4, 2023
@ludoboludo ludoboludo deleted the gift-card-follow-ups branch April 4, 2023 17:44
pangloss added a commit to pangloss/dawn that referenced this pull request Apr 5, 2023
* shopify/main:
  [Video] Add fade in on scroll animation (Shopify#2495)
  Animate rich text and email signup (Shopify#2408)
  [Gift card] Add help doc link and change label for translation (Shopify#2471)
  Add prettier config to support format-on-save (Shopify#2323)
  Announcements slideshow (Shopify#2394)
  fix: Update Follow on Shop Option Text (Shopify#2463)
  Enable "per-PR" async PR mode (Shopify#2488)
  Animate multicolumn (Shopify#2409)
  Fix improper image sizes in the collage section (Shopify#2478)
  Replaced depreciated liquid property/value (Shopify#2480)
pangloss added a commit to pangloss/dawn that referenced this pull request Apr 12, 2023
* next:
  [Video] Add fade in on scroll animation (Shopify#2495)
  Animate rich text and email signup (Shopify#2408)
  [Gift card] Add help doc link and change label for translation (Shopify#2471)
  Add prettier config to support format-on-save (Shopify#2323)
  Announcements slideshow (Shopify#2394)
  fix: Update Follow on Shop Option Text (Shopify#2463)
  Enable "per-PR" async PR mode (Shopify#2488)
  Animate multicolumn (Shopify#2409)
  Fix improper image sizes in the collage section (Shopify#2478)
  Replaced depreciated liquid property/value (Shopify#2480)
  Update from Shopify for theme dawn/next
  Update from Shopify for theme dawn/next
  Update from Shopify for theme dawn/next
  Update from Shopify for theme dawn/next
  Update from Shopify for theme dawn/next
  Update from Shopify for theme dawn/next
phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024
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.

3 participants