-
Notifications
You must be signed in to change notification settings - Fork 11
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
Polished date picker implementation #2195
Conversation
@@ -25,7 +24,7 @@ defmodule Dotcom.TripPlan.InputForm do | |||
typed_embedded_schema do | |||
embeds_one(:from, __MODULE__.Location) | |||
embeds_one(:to, __MODULE__.Location) | |||
field(:datetime_type, Ecto.Enum, values: @time_types) | |||
field(:datetime_type, :string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 I changed this for now because we need to have a way to navigate between these atoms and the string we're given back by the form. We can figure this out later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the atom/string juggling was definitely my least favorite part about trying to handle mode changes too...
@@ -31,7 +31,7 @@ defmodule DotcomWeb.Components.LiveComponents.TripPlannerForm do | |||
assign(socket, %{ | |||
form: form, | |||
location_keys: InputForm.Location.fields(), | |||
show_datepicker: input_value(form, :datetime_type) != :now | |||
show_datepicker: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 This is always false for now until we implement some way to load a plan.
@@ -220,6 +220,12 @@ | |||
} | |||
} | |||
|
|||
#date-picker-calendar { | |||
.form-control[readonly] { | |||
background-color: $body-bg; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 Bootstrap was setting this to grey so we need this override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works well! Some additional polish notes:
- There's a bit too much space between the timepicker and the buttons, and I think it's due to some margins added to all lists via Dotcom's CSS. If you add
mb-0
to the input group's<ul>
tag that'll help. - The timepicker input should have the same border width as the buttons, right now it's a lot thicker
- The calendar icon position is a little wonky, also ought to be blue rather than low-contrast grey
413bfec
to
c45441d
Compare
c45441d
to
c880070
Compare
date_input = ~s(name="input_form[datetime]") | ||
refute html =~ date_input | ||
|
||
html = | ||
view | ||
|> element("input[value=arrive_by]") | ||
|> render_click() | ||
|
||
assert html =~ date_input | ||
|
||
html = | ||
view | ||
|> element("input[value=now]") | ||
|> render_click() | ||
|
||
refute html =~ date_input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 I'm removing these until this stabilizes and we can test the entire form.
html = | ||
view | ||
|> element("form") | ||
|> render_submit() | ||
|
||
assert html =~ "Please add a destination." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📓 Same here. I think we'll want to figure out our final error display approach before writing these tests.
It looks like there are some slight conflicts between the CSS that Cristen did in her polish PR and the changes I made in mine. I figure we can work those out after the big pieces are done being built. Another outstanding task is getting the default date of the date picker to be "now" in East Coast time. This is only a problem for people not on the East Coast, but something we should fix. We can do that in a separate task since it will necessitate a new release of Metro.