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

Add helper text to datetime-local form fields #833

Merged

Conversation

adrianna-chang-shopify
Copy link
Contributor

Currently, datetime form fields are ambiguous because it's unclear whether the application timezone or system timezone will be used. This PR adds helper text to datetime form fields specifying that the system timezone will be used for datetime-local parameter inputs. We can even show the system timezone via Time.now.zone so that it's clearer for debugging purposes (e.g. local dev envs will use the user's local timezone vs prod generally using UTC).

🎩

Screenshot 2023-06-01 at 9 17 37 AM

# Return helper text for the datetime-local form field.
def datetime_field_help_text
tag.div(
tag.p("Datetime inputs will use the system timezone, which is #{Time.now.zone}."),
Copy link
Member

@etiennebarrie etiennebarrie Jun 1, 2023

Choose a reason for hiding this comment

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

This may become incorrect if ActiveModel::Type::DateTime ever handles local times in accordance with Rails.configuration.time_zone/Time.zone. We should add a test for that to make sure we update this if it changes.

e.g. currently, ActiveModel::Type::DateTime.new.cast("2023-06-01T12:00:00") will not respect Time.zone but only the system time zone.

We could write a test that changes Time.zone to something that's not Time.now.zone, then tries to parse a local time as sent by a browser and checks that the timezone is correct.

Copy link
Member

Choose a reason for hiding this comment

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

Actually it's already incorrect if the application is not setting config.time_zone. In this case, it will always use UTC. If config.time_zone is set, it won't use it but use the system time zone instead… It will always ignore Time.zone though.

@etiennebarrie etiennebarrie force-pushed the ac-timezone-text-helper-on-datetime-inputs branch 2 times, most recently from 4a28350 to 914af37 Compare June 7, 2023 12:53
adrianna-chang-shopify and others added 2 commits June 7, 2023 16:53
Currently, datetime form fields are ambiguous because it's unclear whether
the application timezone or system timezone will be used. This commit
adds helper text to datetime form fields specifying which timezone will
be used for datetime-local parameter inputs.

Co-authored-by: Étienne Barrié <etienne.barrie@shopify.com>
@etiennebarrie etiennebarrie force-pushed the ac-timezone-text-helper-on-datetime-inputs branch from 914af37 to 189fd85 Compare June 7, 2023 14:54
@adrianna-chang-shopify
Copy link
Contributor Author

@etiennebarrie is this ready to merge?

@etiennebarrie
Copy link
Member

Yes I think so!

@adrianna-chang-shopify
Copy link
Contributor Author

@etiennebarrie can you approve it and then I'll merge? 😅

@adrianna-chang-shopify adrianna-chang-shopify merged commit eeee3f4 into main Jun 13, 2023
@adrianna-chang-shopify adrianna-chang-shopify deleted the ac-timezone-text-helper-on-datetime-inputs branch June 13, 2023 13:01
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.

2 participants