Add format option to time field #2029
Conversation
tjvc
force-pushed
the
add-format-to-time
branch
2 times, most recently
from
cfbe6a7 to
105383f
Compare
|
@pablobm @amymariewall I think this is ready for another review. I've addressed the linter errors and another issue I spotted: because I18n uses the same key in the locales file for all objects with a time ( |
spec/lib/fields/time_spec.rb
Outdated
| formats: { short: "%H:%M" }, | ||
| }, | ||
| } | ||
| end |
There was a problem hiding this comment.
A minor point: we typically avoid using let in this codebase and prefer to either inline a variable in each it or extract it into a normal Ruby method.
spec/lib/fields/time_spec.rb
Outdated
| describe "#time" do | ||
| before do | ||
| @time = DateTime.new(2021, 3, 26, 16, 38) | ||
| end |
There was a problem hiding this comment.
And before! I'd just put the DateTime definition in each it block.
There was a problem hiding this comment.
Sorry, I totally missed the second part of your original comment! 🤦♂️
Previously there was no way to specify format for a time field, but you can specify a format for date and datetime fields. This commit adds the ability to pass a format option to time fields. Because I18n uses the time locale key for for all objects with a time [0], retain the hard-coded time field format where no format option is passed to avoid a breaking change. Addresses issue #1227. [0] https://github.com/ruby-i18n/i18n/blob/0888807ab2fe4f4c8a4b780f5654a8175df61feb/lib/i18n/backend/base.rb#L82 Co-authored-by: Thom Carter <thom.carter@thoughtbot.com>
tjvc
force-pushed
the
add-format-to-time
branch
from
93fc827 to
55843cc
Compare
Previously there was no way to specify format for a time field, but you can specify a format for date and datetime fields. This commit adds the ability to pass a format option to time fields.
Because I18n uses the time locale key for for all objects with a time, retain the hard-coded time field format where no format option is passed to avoid a breaking change.
Addresses #1227.