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

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

Merged
merged 1 commit into from Jan 17, 2020

Conversation

@crisbeto
Copy link
Member

crisbeto commented Jan 12, 2020

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:
demo

@crisbeto crisbeto requested a review from mmalerba as a code owner Jan 12, 2020
@googlebot googlebot added the cla: yes label Jan 12, 2020
@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch 2 times, most recently from e3fd3e1 to ab5ff5a Jan 12, 2020
@mmalerba

This comment has been minimized.

Copy link
Contributor

mmalerba commented Jan 12, 2020

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

This comment has been minimized.

Copy link
Member Author

crisbeto commented Jan 12, 2020

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

This comment has been minimized.

Copy link
Contributor

mmalerba commented Jan 13, 2020

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 crisbeto changed the base branch from date-selection-model-new to date-range Jan 13, 2020
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Jan 13, 2020

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

@ccjmne

This comment has been minimized.

Copy link

ccjmne commented Jan 13, 2020

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

I have two remarks, however:

  1. 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!

  2. 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 🙂.

@mmalerba

This comment has been minimized.

Copy link
Contributor

mmalerba commented Jan 13, 2020

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch from ab5ff5a to bb0f60a Jan 13, 2020
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Jan 13, 2020

@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 Outdated Show resolved Hide resolved
}
}

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

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 13, 2020

Member

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?

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 13, 2020

Author Member

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

11/11     - 12/12

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 13, 2020

Member

What if we right-align the the input?

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 13, 2020

Author Member

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.

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 13, 2020

Member

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.

This comment has been minimized.

Copy link
@mmalerba

mmalerba Jan 16, 2020

Contributor

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

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 17, 2020

Author Member

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.

This comment has been minimized.

Copy link
@mmalerba

mmalerba Jan 17, 2020

Contributor

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

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 17, 2020

Author Member

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

This comment has been minimized.

Copy link
@crisbeto

crisbeto Jan 17, 2020

Author Member

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch from bb0f60a to cc66e6f Jan 13, 2020
src/dev-app/datepicker/datepicker-demo.html Outdated Show resolved Hide resolved
}
}

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

This comment has been minimized.

Copy link
@mmalerba

mmalerba Jan 14, 2020

Contributor

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch from cc66e6f to 73d4695 Jan 15, 2020
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Jan 15, 2020

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch from 73d4695 to 6445c36 Jan 15, 2020
Copy link
Member

jelbourn left a comment

Overall looks good, just a few more minor comments

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

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

This comment has been minimized.

Copy link
@jelbourn

jelbourn Jan 16, 2020

Member

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch from 6445c36 to 781a535 Jan 16, 2020
@crisbeto

This comment has been minimized.

Copy link
Member Author

crisbeto commented Jan 16, 2020

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

@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch 2 times, most recently from a0c6c2e to 60aa620 Jan 17, 2020
@crisbeto crisbeto force-pushed the crisbeto:date-selection-model-new branch 2 times, most recently from d371811 to 8444614 Jan 17, 2020
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 crisbeto force-pushed the crisbeto:date-selection-model-new branch from 8444614 to 5780d04 Jan 17, 2020
Copy link
Member

jelbourn left a comment

LGTM

@jelbourn jelbourn merged commit 5ee2173 into angular:date-range Jan 17, 2020
12 checks passed
12 checks passed
ci/angular: merge status All checks passed!
ci/circleci: api_golden_checks Your tests passed on CircleCI!
Details
ci/circleci: bazel_build Your tests passed on CircleCI!
Details
ci/circleci: build_release_packages Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: ngcc_compatibility Your tests passed on CircleCI!
Details
ci/circleci: tests_browserstack Your tests passed on CircleCI!
Details
ci/circleci: tests_local_browsers Your tests passed on CircleCI!
Details
ci/circleci: tests_saucelabs Your tests passed on CircleCI!
Details
ci/circleci: view_engine_test Your tests passed on CircleCI!
Details
cla/google All necessary CLAs are signed
@crisbeto crisbeto deleted the crisbeto:date-selection-model-new branch Jan 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.