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

Date and Time fields #931

Closed
wants to merge 14 commits into from

Conversation

bronislav
Copy link

@bronislav bronislav commented Jul 14, 2017

This adds date and time fields. #741

Continuation of #757

@@ -174,7 +174,7 @@ class User < ActiveRecord::Base
end
end

it "assigns dates, times, and datetimes a type of `DateTime`" do
it "assigns dates, times, and datetimes a type of `Date`, `Time` and `DateTime`" do

Choose a reason for hiding this comment

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

Line is too long. [89/80]

end

def select_from_timepicker(time_string)
page.execute_script(<<-JS)

Choose a reason for hiding this comment

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

Trailing whitespace detected.

@bronislav bronislav changed the title Date and Time fields [WIP] Date and Time fields Jul 14, 2017
@bronislav bronislav mentioned this pull request Jul 14, 2017
@bronislav bronislav changed the title [WIP] Date and Time fields Date and Time fields Jul 14, 2017
Copy link
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

This is looking good!

There's a minor test failure where the date/time isn't displaying quite right, and I've left a few other comments.

Do you think you could add the new fields to the /docs, too?

@@ -0,0 +1,5 @@
class AddBirthdateToCustomer < ActiveRecord::Migration[4.2]
Copy link
Member

Choose a reason for hiding this comment

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

Could you set the migration to 5.1? We've been doing that for recent migrations.

@@ -0,0 +1,5 @@
class AddExampleTimeToCustomer < ActiveRecord::Migration[4.2]
def change
add_column :customers, :example_time, :time
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about doing birth_date and birth_time to be consistent?

(It's the closest I can think which might make sense and avoid us having an example_time field.)

@@ -0,0 +1,48 @@
require "rails_helper"
require "administrate/field/date"
# require "support/field_matchers"
Copy link
Member

Choose a reason for hiding this comment

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

Could we lose this comment?

@bronislav bronislav force-pushed the ai-date-and-time-fields branch from 6f310f4 to 3ee0a16 Compare July 19, 2017 09:14
formats: {
default: "%r",
short: "%H:%M",
time_without_date: "%H:%M:%S"

Choose a reason for hiding this comment

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

Put a comma after the last item of a multiline hash.

@nickcharlton
Copy link
Member

This is looking good! Do you think you'd also be able to contribute some docs, too? We've got a list of included fields which this should be part of in docs/customizing_dashboards.md.

@bronislav
Copy link
Author

@nickcharlton Sure. I will update docs

There is still one failing spec with which I'm fighting. It's related to the timezone handling. I plan to fix it this weekend.

@bronislav
Copy link
Author

@nickcharlton I've added fields to the docs, but there is still one issue with date picker. Datepicker tries to show time in the local timezone when original value is in UTC.

@nickcharlton
Copy link
Member

Is that because the time being stored doesn't have a time zone, or is it losing the timezone? Or, is it just doing the Rails thing of assuming you'd want it in a local time?

I think as a user I'd expect it to display in the same way I'd stored it, but that makes it difficult if we're not storing that data.

@bronislav
Copy link
Author

bronislav commented Jul 28, 2017

@nickcharlton We are storing it without timezone and display on the form in the UTC. But, datetime_picker shows it in the local timezone, which brings inconsistency.

From my perspective, I'd say that we don't need to save timezone and show exactly save value as saved. I wasn't managed to configure datetime_picker to not add timezone to the value. Still trying.

@bronislav
Copy link
Author

@nickcharlton I won't work on this in the nearest feature. I don't use Rails for a while now. If anyone would like to take ownership on this or feel free to just close it.

@bronislav bronislav closed this Apr 1, 2019
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.

4 participants