-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Add datepicker for scheduling gift cards #2540
Conversation
We're using a native date picker for now as this was deemed to be the best starting point int terms of utility and accessibility. As the feature matures we may choose to implement something custom which is more themable but this is a good starting position. Other theme developers may use a different date picker as long as the line itmem property is in ISO format YYYY-MM-DD We also now use error messages which contain the field as part of the error string which is much better for localisation. Unfortunately it would be a breaking change to introduce this, so we've made it opt in my keying on the new _shopify_offset parameter. It's a bit strange to use this property as the switch but it works. We want the TZ offset so that we can schedule the gift card send for a sensible time in the user's local timezone rather than in the shop timezone which might cause confusion. Whilst I was here I also cleaned up a minor bug with empty line item properties being sent even when the checkbox was not checked. I've done this simply by disabling the fields whenever the checkbox is unchecked.
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.
Good work!
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.
A few comments, mostly nipticks. A few questions as well to make sure I understand the context 🙂
Nothing blocking. 👍
@@ -106,7 +105,6 @@ | |||
{% render 'icon-error' %} | |||
<span class="error-message"> | |||
{%- if form.errors contains 'email' -%} | |||
{{ form.errors.translated_fields.email | capitalize }} |
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.
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.
So this is an temporary edge case that you should only see if javascript is disabled. That's clearly not the case in this screenshot so very interested in a replication for how you got this.
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.
- So for this I pulled your branch locally and used the Shopify CLI to show me what it looked like (
shopify theme dev
) on our test store. - Then I enabled the gift card recipient form
- And when I click on add to cart after checking the
I want to send this as gift
box, that what it shows me.
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.
Oh that's fine, the backend that supports the new error messages isn't merged yet. It should look better in the spin links above.
For some more context we originally just copied what the customer form does which is concatenate error messages with their field name in liquid like {{ name }}{{ message }}
which we knew was bad but we were pressed for time so we didn't mess with the precedent. Now we have some time to revaluate we think it's pretty unacceptable to do this so we're fixing the messages so they are localised properly. That would be a breaking change if not handled properly so we're using the presence of the offset field as a hint the theme can cope with the new better error strings.
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 requested translations 👍 so we will have to wait on them before being able to merge
* Add datepicker for scheduling gift cards We're using a native date picker for now as this was deemed to be the best starting point int terms of utility and accessibility. As the feature matures we may choose to implement something custom which is more themable but this is a good starting position. Other theme developers may use a different date picker as long as the line itmem property is in ISO format YYYY-MM-DD We also now use error messages which contain the field as part of the error string which is much better for localisation. Unfortunately it would be a breaking change to introduce this, so we've made it opt in my keying on the new _shopify_offset parameter. It's a bit strange to use this property as the switch but it works. We want the TZ offset so that we can schedule the gift card send for a sensible time in the user's local timezone rather than in the shop timezone which might cause confusion. Whilst I was here I also cleaned up a minor bug with empty line item properties being sent even when the checkbox was not checked. I've done this simply by disabling the fields whenever the checkbox is unchecked. * Rename recipient fields for consistency * invert if/else for readibilty * Update 20 translation files * Update 10 translation files --------- Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>
PR Summary:
Adds date picker for scheduling gift card sending for a future date.
Why are these changes introduced?
Closes a great number of merchant frustrations
What approach did you take?
We're using a native date picker for now as this was deemed to be the best starting point int terms of utility and accessibility. As the feature matures we may choose to implement something custom which is more themable but this is a good starting position.
Other theme developers may use a different date picker as long as the line itmem property is in ISO format YYYY-MM-DD
We also now use error messages which contain the field as part of the error string which is much better for localisation. Unfortunately it would be a breaking change to introduce this, so we've made it opt in my keying on the new _shopify_offset parameter. It's a bit strange to use this property as the switch but it works.
We want the TZ offset so that we can schedule the gift card send for a sensible time in the user's local timezone rather than in the shop timezone which might cause confusion.
Whilst I was here I also cleaned up a minor bug with empty line item properties being sent even when the checkbox was not checked. I've done this simply by disabling the fields whenever the checkbox is unchecked.
Other considerations
Decision log
Visual impact on existing themes
Testing steps/scenarios
Demo links
Checklist