feat(datepicker): set up input for date range picker #18159

crisbeto · 2020-01-12T14:48:16Z

Sets up the UI and most of the boilerplate that we'll need for the input that is associated with a date range picker. Doesn't include any interactions with the datepicker itself yet.

Example of how it works:

mmalerba · 2020-01-12T17:05:25Z

Can you send this PR to the date-range branch I just created instead? I'd like to keep it separate, because this part is more of a new feature, whereas the stuff currently on date-selection-model-new is a refactoring. If we keep it separate, we can work on getting the refactoring merged while the feature is in development.

crisbeto · 2020-01-12T17:22:22Z

I'm not sure whether I'd be able to continue working on separately from here on out. The rest of the UI all depends on the date selection model being in place. Maybe now is the time to try and get the selection model merged in?

mmalerba · 2020-01-13T04:59:41Z

Yeah I created that branch at the same commit as this one. You can work on that branch while we try to get this one in master

crisbeto · 2020-01-13T06:46:19Z

Cool, I've changed the PR to point to the date-range branch.

ccjmne · 2020-01-13T15:58:01Z

This is fantastic and looks exactly like what I imagined. Thank you very much!

I have two remarks, however:

The dash used to denote ranges should be the "en-dash" (–), and I see we are using the "em-dash" here by default. I realise it merely is the default value for the @Input parameter, but I reckon we might as well use the more correct option!
From the GIF you posted, it looks as if the behaviour of this form-field is a little different from that of the simpler ones, where having both a label and a placeholder text only shows the label so long as the field isn't focused nor valued (assuming floatLabel other than "always" — which seems to be the case).
For reference, the field that uses a custom input control (last item in the form-field examples) does float the label "back and forth" as you focus in and blur out of either of the three backing inputs.

Maybe I'm overlooking some things though, I haven't even run the code and am just doing my daily quick scan of what's happening 🙂.

src/material/datepicker/date-range-input.ts

mmalerba · 2020-01-13T18:48:38Z

I agree with @ccjmne that we should not force the label to always float, we can just hide the placeholders and dash when the label isn't floating

src/dev-app/datepicker/datepicker-demo.html

src/material/datepicker/date-range-input.scss

crisbeto · 2020-01-13T20:38:37Z

@mmalerba all of the feedback should be addressed, aside from the question about what the public API should look like.

src/dev-app/datepicker/datepicker-demo.html

src/material/datepicker/date-range-input.scss

jelbourn · 2020-01-13T19:26:39Z

src/material/datepicker/date-range-input.scss

+  }
+}
+
+// We want the start input to be flush against the separator, no matter how much text it has, but


Why does the input need to be flush against the separator?

Looking at the gif of your demo, I think it would be better if the separate and end-date input didn't shift as you were typing into the start date. Would it be better to give the input a min-width (equivalent to e.g. "77/77/7777") so that the end date input doesn't shift as the user is typing?

Because it looks weird if you typed something shorter and then the space around the separator was uneven, e.g.

11/11 - 12/12

What if we right-align the the input?

Which one? The start date input or the date range input in general? I think we'd just the same problem but in a different place.

I pulled down the PR and tried tweaking a few things locally. I concluded that everything I suggested looks worse than what you ultimately did. I have one more idea, though, which is to hide the end-date input placeholder once you start typing into the start-date input. I tried this out locally and I think it eliminates the appearance of the input getting pushed.

Please leave TODO's for anything that you will handle in a follow-up PR. I feel we've been getting a little lax with the "will handle in a follow-up PR" lately, and I'm worried that things might slip off our radar and never actually get done

I ended up just doing it now. Can you take one more look @mmalerba? The way it behaves now is that it hides the placeholders when one of these criteria is true:

The inputs are empty and they have focus.

The start input is focused and the end input is empty.

The way I was imagining it, the dash doesn't disappear. @jelbourn should take a look too, since he felt more strongly about the placeholder stuff

The way I was reading it the dash should disappear, otherwise you'll see it jump to the beginning when you start typing.

I've updated it to keep the separator visible while the start has a value.

src/material/datepicker/date-range-input.scss

src/dev-app/datepicker/datepicker-demo.html

mmalerba · 2020-01-14T16:39:30Z

src/material/datepicker/date-range-input.scss

+  }
+}
+
+// We want the start input to be flush against the separator, no matter how much text it has, but


We should also think about the edge cases related to this. e.g. what happens if I skip the start and type in the end input?

If we want to go with Jeremy's suggestion my mental model would be to treat them as a single placeholder, i.e. whenever one of the inputs has text in it both placeholders are hidden

src/material/datepicker/date-range-input.html

crisbeto · 2020-01-15T21:35:52Z

@mmalerba @jelbourn I've reworked it to use two projected inputs. Can you take another look?

jelbourn

Overall looks good, just a few more minor comments

src/material/datepicker/date-range-input-parts.ts

src/material/datepicker/date-range-input.scss

jelbourn · 2020-01-16T00:09:21Z

src/material/datepicker/date-range-input.scss

+  }
+}
+
+// We want the start input to be flush against the separator, no matter how much text it has, but


Agreed that we should treat them conceptually as one placeholder. Do you want to do that in this PR or in a follow-up?

src/material/datepicker/date-range-input.scss

crisbeto · 2020-01-16T07:14:58Z

Addressed the latest set of feedback. Also fixed that the error state wasn't being calculated correctly.

Sets up the UI and most of the boilerplate that we'll need for the input that is associated with a date range picker. Doesn't include any interactions with the datepicker itself yet.

jelbourn

LGTM

Sets up the UI and most of the boilerplate that we'll need for the input that is associated with a date range picker. Doesn't include any interactions with the datepicker itself yet.

angular-automatic-lock-bot · 2020-02-18T00:02:54Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

crisbeto added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent pr: merge safe labels Jan 12, 2020

crisbeto requested a review from mmalerba as a code owner January 12, 2020 14:48

crisbeto assigned jelbourn and mmalerba Jan 12, 2020

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 12, 2020

crisbeto force-pushed the date-selection-model-new branch 2 times, most recently from e3fd3e1 to ab5ff5a Compare January 12, 2020 15:26

crisbeto changed the base branch from date-selection-model-new to date-range January 13, 2020 06:45

ccjmne reviewed Jan 13, 2020

View reviewed changes

src/material/datepicker/date-range-input.ts Outdated Show resolved Hide resolved

mmalerba reviewed Jan 13, 2020

View reviewed changes

src/dev-app/datepicker/datepicker-demo.html Outdated Show resolved Hide resolved

src/material/datepicker/date-range-input.scss Show resolved Hide resolved

crisbeto force-pushed the date-selection-model-new branch from ab5ff5a to bb0f60a Compare January 13, 2020 20:38

jelbourn reviewed Jan 13, 2020

View reviewed changes

crisbeto force-pushed the date-selection-model-new branch from bb0f60a to cc66e6f Compare January 13, 2020 21:36

mmalerba reviewed Jan 14, 2020

View reviewed changes

crisbeto force-pushed the date-selection-model-new branch from cc66e6f to 73d4695 Compare January 15, 2020 21:34

crisbeto force-pushed the date-selection-model-new branch from 73d4695 to 6445c36 Compare January 15, 2020 21:39

jelbourn reviewed Jan 16, 2020

View reviewed changes

crisbeto force-pushed the date-selection-model-new branch from 6445c36 to 781a535 Compare January 16, 2020 07:14

crisbeto force-pushed the date-selection-model-new branch 2 times, most recently from a0c6c2e to 60aa620 Compare January 17, 2020 19:02

mmalerba approved these changes Jan 17, 2020

View reviewed changes

crisbeto force-pushed the date-selection-model-new branch 2 times, most recently from d371811 to 8444614 Compare January 17, 2020 21:11

feat(datepicker): set up input for date range picker

5780d04

Sets up the UI and most of the boilerplate that we'll need for the input that is associated with a date range picker. Doesn't include any interactions with the datepicker itself yet.

crisbeto force-pushed the date-selection-model-new branch from 8444614 to 5780d04 Compare January 17, 2020 21:17

jelbourn approved these changes Jan 17, 2020

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 17, 2020

jelbourn merged commit 5ee2173 into angular:date-range Jan 17, 2020

crisbeto deleted the date-selection-model-new branch January 18, 2020 13:54

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 18, 2020

feat(datepicker): set up input for date range picker #18159

feat(datepicker): set up input for date range picker #18159

Uh oh!

Conversation

crisbeto commented Jan 12, 2020

Uh oh!

mmalerba commented Jan 12, 2020

Uh oh!

crisbeto commented Jan 12, 2020

Uh oh!

mmalerba commented Jan 13, 2020

Uh oh!

crisbeto commented Jan 13, 2020

Uh oh!

ccjmne commented Jan 13, 2020

Uh oh!

Uh oh!

mmalerba commented Jan 13, 2020

Uh oh!

Uh oh!

Uh oh!

crisbeto commented Jan 13, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto Jan 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crisbeto commented Jan 15, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

crisbeto commented Jan 16, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Feb 18, 2020

Uh oh!

Uh oh!

crisbeto Jan 13, 2020 •

edited

Loading