Skip to content

Refactor Time-related Fields ahead of upcoming improvements#2702

Closed
goosys wants to merge 2 commits intothoughtbot:mainfrom
goosys:refactor-datetime-format
Closed

Refactor Time-related Fields ahead of upcoming improvements#2702
goosys wants to merge 2 commits intothoughtbot:mainfrom
goosys:refactor-datetime-format

Conversation

@goosys
Copy link
Copy Markdown
Contributor

@goosys goosys commented Nov 7, 2024

I initially planned to improve the i18n for Time-related fields, but I realized some preliminary refactoring was necessary. I’ve made a few small adjustments and would appreciate your review.

  • Ensured consistency across public methods for DateTime, Date, and Time, which each had slight differences.
  • Updated Field::Time#time to always go through I18n.localize:
  • Removed the default option from I18n.localize:
    • Since an exception is thrown with data.in_time_zone when data is nil, the default option was not meaningful.
  • Removed the default value for timezone and removed string conversion with .name:
    • Time.zone is always defined, so a default value is unnecessary.
    • in_time_zone can accept Time.zone objects directly, so string conversion is also unnecessary.
  • Added Customer.example_time to the Dashboard in the example app to check the behavior of the Field::Time .
  • Added tests
    • Added a test to confirm that the existing behavior remains intact.
    • Also added a test for cases where the am/pm translations are undefined.
      • Since our application operates in non-English locales and always includes the rails-i18n gem, we don’t have to worry about am/pm translations being undefined.

Next, I’d like to make the following adjustments. What do you think?

  • Set options.fetch(:format, :administrate_datetime_default) as the default format value
    • This way, we avoid modifying the :default format used in the main application outside of the Administrate panel.
    • Currently, the Time format is fixed as "%I:%M%p", but in Japanese, we would prefer the order "%p %I:%M". I believe other languages may also have their own preferred orders.
  • To use different formats in index and show views, I’d like to allow the format option to be passed into public methods.
    • In the index view, this would allow using field.time(format: :administrate_time_short), while in the show view, simply calling field.time would apply :administrate_time_default.

@nickcharlton
Copy link
Copy Markdown
Member

Thanks! These changes all look sensible to me. Could you break them out into individual commits (and so PRs)?

Set options.fetch(:format, :administrate_datetime_default) as the default format value

Where would :administrate_datetime_default come from?

@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 18, 2025

@nickcharlton

Could you break them out into individual commits (and so PRs)?

I've done that. Could you review them again?

Where would :administrate_datetime_default come from?

We can enable this by configuring it in i18n.
Here is a sample: goosys@7f492e9

There are two possible approaches:

  1. Reusing administrate_default for datetime and date, as shown in the sample.
  2. Defining separate keys for administrate_datetime_default, administrate_date_default, and administrate_time_default.

If we choose to define all of them separately, it would look like the following (though I haven't tested it yet).

module Administrate
  module Field
    class DateTime < Base
      def date
        I18n.localize(
          data.in_time_zone(timezone).to_date,
          format: format_date
        )
      end

      def datetime
        I18n.localize(
          data.in_time_zone(timezone),
          format: format_datetime
        )
      end

      private

      def format_date
        options.fetch(:format, :administrate_date_default)
      end

      def format_datetime
        options.fetch(:format, :administrate_datetime_default)
      end

@goosys
Copy link
Copy Markdown
Contributor Author

goosys commented Feb 19, 2025

Thank you.

I have created a new PR for the next task, so I will close this one.
#2781

@goosys goosys closed this Feb 19, 2025
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