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

Added Time Field #903

Merged
merged 5 commits into from Mar 2, 2018

Conversation

@arcticbarra
Copy link
Contributor

arcticbarra commented May 26, 2017

Added a time field to administrate. Instead of showing both the calendar and the time picker it only shows the time picker as shown in the screenshot. It's based on my administrate-field-time gem
screenshot 2017-05-25 23 29 41

@arcticbarra arcticbarra force-pushed the arcticbarra:time-field branch 4 times, most recently from 4ddb4a4 to 5661087 May 26, 2017
describe "#to_partial_path" do
it "returns a partial based on the page being rendered" do
page = :show
field = Administrate::Field::Time.new(:time, Time.zone.local(2000, 1, 1, 15, 45, 33), page)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 26, 2017

Line is too long. [97/80]

@@ -168,7 +168,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 `DateTime` and `Time`" do

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot May 26, 2017

Line is too long. [81/80]

@arcticbarra arcticbarra force-pushed the arcticbarra:time-field branch 5 times, most recently from d1f2701 to f94d5aa May 26, 2017
@@ -1,4 +1,8 @@
$(function () {
$(".timepicker").datetimepicker({
debug: false,
format: "HH:mm:ss",

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO May 27, 2017

Collaborator

indentation

page = :show
field = Administrate::Field::Time.new(:time,
Time.zone.local(2000, 1,
1, 15, 45, 33),

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO May 27, 2017

Collaborator

maybe just move this to a variable :p

@@ -10,7 +10,7 @@ class DashboardGenerator < Rails::Generators::NamedBase
enum: "Field::String",
float: "Field::Number",
integer: "Field::Number",
time: "Field::DateTime",

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO May 27, 2017

Collaborator

Is it a good idea to remove the DateTime here?

This comment has been minimized.

Copy link
@arcticbarra

arcticbarra May 27, 2017

Author Contributor

In my opinion yes, because when we create a time attribute we want the Field::Time instead of the current Field::DateTime

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO May 27, 2017

Collaborator

I was more thinking along the lines of adding a datetime here and being able to specify datetime in the generators; at least I think that's what this file does if I remember right.

This comment has been minimized.

Copy link
@arcticbarra

arcticbarra May 27, 2017

Author Contributor

So being able to decide if we want our time attribute to be of type time or datetime? By default generating time attributes as datetime and if the user wants them to be time change them in the dashboard?

@@ -1,4 +1,8 @@
$(function () {
$(".timepicker").datetimepicker({

This comment has been minimized.

Copy link
@tysongach

tysongach May 27, 2017

Member

Can we target the element using a data attribute instead of a class?

@arcticbarra arcticbarra force-pushed the arcticbarra:time-field branch 4 times, most recently from de61e38 to 5a4eb6f May 27, 2017
debug: false,
format: "HH:mm:ss",
});
$('[data-type="time"]').datetimepicker({

This comment has been minimized.

Copy link
@BenMorganIO

BenMorganIO May 27, 2017

Collaborator

This looks like it's in the wrong order. Shouldn't this be datetime and the above be time?

This comment has been minimized.

Copy link
@arcticbarra

arcticbarra May 27, 2017

Author Contributor

Whoops, you're right, let me change it

@arcticbarra

This comment has been minimized.

Copy link
Contributor Author

arcticbarra commented May 28, 2017

@BenMorganIO All other changes have been made, I'm just waiting for your response on the time/datetime comment 👍

@acrogenesis

This comment has been minimized.

Copy link
Contributor

acrogenesis commented Jun 19, 2017

Any updates on this merge?

arcticbarra added 4 commits May 26, 2017
@arcticbarra arcticbarra force-pushed the arcticbarra:time-field branch from 02ddc75 to 5ea09bc Jul 15, 2017
@arcticbarra

This comment has been minimized.

Copy link
Contributor Author

arcticbarra commented Jul 15, 2017

Just rebased the branch to be up to date.

@nickcharlton

This comment has been minimized.

Copy link
Member

nickcharlton commented Jul 17, 2017

I'm currently waiting on @BenMorganIO's input on here; which I'm sure they'll get to when they're able soon.

@m5o

This comment has been minimized.

Copy link

m5o commented Jan 22, 2018

Friendly ping 🔔 to @BenMorganIO

@nickcharlton

This comment has been minimized.

Copy link
Member

nickcharlton commented Mar 2, 2018

I think we're good, so I'm going to go ahead and merge this.

@nickcharlton nickcharlton merged commit d7d8db5 into thoughtbot:master Mar 2, 2018
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
hound No violations found. Woof!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.