[DatePicker] allow year selecton to be open before month/day selection #6420

3upzorz · 2017-03-23T02:08:39Z

PR has tests / docs demo, and is linted.
Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

The DatePicker component does not have the ability to open the year selection by default, this PR allows a user to specify an attribute to allow the DatePicker to open to year by default.

Closes #4949

mbrookes · 2017-03-24T00:30:45Z

Given this prop only has two options, it might make more sense to make it a boolean.

oliviertassinari · 2017-03-24T00:35:39Z

@mbrookes I was wondering: #4949 (comment)

oliviertassinari · 2017-03-24T00:36:01Z

src/DatePicker/Calendar.spec.js

+
+      assert.strictEqual(wrapper.find(CalendarMonth).length, 1, 'should have the calendar month select');
+    });
+    it('should display the year pick view when initialView is set to year', () => {


missing blank line

oliviertassinari · 2017-03-24T00:36:55Z

src/DatePicker/Calendar.spec.js

@@ -223,4 +225,28 @@ describe('<Calendar />', () => {
      assert.strictEqual(wrapper.props().style.width, 310, 'should not allow space for date display');
    });
  });
+
+  describe('Initial State', () => {


We use description starting with a lower case. It's simply a convention.

mbrookes · 2017-03-24T01:07:42Z

@oliviertassinari I see your comment, and the one you replied to, but not any supporting argument.

A boolean prop would allow <DatePicker openOnYear /> , (or whatever the prop is called), whereas an enum has to be specified.

3upzorz · 2017-03-24T01:42:51Z

@mbrookes having an enum could allow for more than two options in the future, for example if a view for selecting the month was introduced.

mbrookes · 2017-03-24T01:57:33Z

@3upzorz Not going to happen. 😄 (Not on master at least.)

3upzorz · 2017-03-24T03:15:43Z

@mbrookes @oliviertassinari so should I change to match the boolean approach?

oliviertassinari · 2017-03-24T09:12:56Z

Pick what @mbrookes think is best, his point is valid 👍.

oliviertassinari

Actually, aside from using a boolean over an enum, I think that we should be

using the context to propagate the value
making is controllable (stateless) instead of impacting the original state then having no more control on the value.

3upzorz · 2017-03-27T03:28:29Z

@oliviertassinari

Why do you recommend using context? Seeing as there are other props passed similarly, this approach seemed to make the most sense.
Why would this feature need to be controllable? The purpose of this PR is to allow a developer to use a prop to set the initial state of the Calendar component. After the initial state is set, that state value should be controlled as it normally is, no?

I can change to use a boolean prop, but I don't know about these two points. Maybe I'm missing something? It would be great if you could provide some more clarification on your points or some guidance on how to implement your suggestions.

oliviertassinari · 2017-03-27T07:10:53Z

Why do you recommend using context?

@3upzorz I was suggesting using the context as we have to pass the property from parent component to child component multiple times. Yes, that would be the first introduction of the context for that component. Hence, we would break consistency. That's just a suggestion, I think that we should use it in the rewrite on the next branch. That would allow higher flexibility, we would be able to more simply break the component into different chunks.

Why would this feature need to be controllable?

Because it's providing more flexibility. If my memory is correct, we have an issue where someone is asking for it. But I could be wrong. Extra flexibility for use cases that don't exist also means more complexity. Let me check that.

oliviertassinari · 2017-03-27T18:34:17Z

@3upzorz From reading the #4949 thread, it feels like that @mbrookes and you are correct.
a boolean property like openToYearSelection false by default only affecting the initial opening state sounds good.

oliviertassinari

A small comment. I haven't tested the code locally but that looks good otherwise 👍 .

oliviertassinari · 2017-03-29T12:12:28Z

src/DatePicker/DatePicker.js

@@ -119,6 +119,10 @@ class DatePicker extends Component {
     */
    onTouchTap: PropTypes.func,
    /**
+     * If true sets the datepicker to open to year selection first


Missing point at the end of the description.

Still needs to be fixed.

ok

3upzorz · 2017-03-30T07:02:55Z

@oliviertassinari so what's the status of this PR? Any plans to merge it?

mbrookes · 2017-03-30T12:49:45Z

docs/src/app/components/pages/components/DatePicker/ExampleSimple.js

@@ -4,12 +4,14 @@ import DatePicker from 'material-ui/DatePicker';
 /**
 * The Date Picker defaults to a portrait dialog. The `mode` property can be set to `landscape`.
 * You can also disable the Dialog passing `true` to the `disabled` property.
+ * To display the year selection first, set the `initialView` property to "year".


Please can you update this wording?

mbrookes · 2017-03-31T18:53:53Z

I have a small concern regarding usability: if you click a year, it switches to the month/day view, but if you accept the current year with "OK" it closes the dialog; you need to know to click the date in the DateDisplay to switch to date input.

Not sure if that's a topic for this or a separate PR?

3upzorz · 2017-04-05T00:48:05Z

@mbrookes seems like a topic for a separate PR. That's more a question of, do you want the OK button's behaviour to change across different views that are displayed.

oliviertassinari · 2017-04-05T07:27:07Z

That's more a question of, do you want the OK button's behaviour to change across different views that are displayed

@3upzorz That sounds like a good use case for a controlled property that show or hide the year mode. So I think that it's linked to that PR. User could either want to change the view at the year selection event or at the presse OK button event. What do you think?

3upzorz · 2017-04-06T03:06:18Z

@oliviertassinari What is the use case for being able to set this as a controlled property? Having to control this property manually would make the component more painful to use as you would essentially have to handle part of the state of this component in your own application.

oliviertassinari · 2017-04-08T23:08:02Z

What is the use case for being able to set this as a controlled property?

The use case is what @mbrookes suggested. You might want to implement a custom view display logic. My only concern is around, does user have access to the event to actually control the property? Does they know when the year is clicked, when the OK button is clicked, when a year is selected?

would make the component more painful to use as you would essentially have to handle part of the state of this component in your own application.

Isn't the whole point of overriding the default behavior? If you do so, you have the responsibility to make it better.

oliviertassinari · 2017-05-21T08:36:58Z

I'm closing the PR has as been inactive for quite some time.

davidhantson · 2017-06-12T10:10:41Z

Has this feature now been implemented or not? I have a case which requests me to either style the "Year" to be highlighted or start the year mode by default.

oliviertassinari · 2017-06-12T10:12:30Z

@davidhantson It hasn't been implemented.

[DatePicker] allow year selecton to be open before month/day selection

acfe1ba

muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 23, 2017

[DatePicker] fix lint errors

d5339aa

muibot added the PR: Needs Review label Mar 23, 2017

oliviertassinari approved these changes Mar 23, 2017

View reviewed changes

oliviertassinari added component: dialog This is the name of the generic UI component, not the React module! and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Mar 23, 2017

oliviertassinari reviewed Mar 24, 2017

View reviewed changes

[DatePicker] lowercase test description, add blank line

02af6f6

muibot added the PR: Needs Review label Mar 24, 2017

oliviertassinari previously requested changes Mar 26, 2017

View reviewed changes

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Mar 26, 2017

change prop name to openToYearSelection and make a boolean

42b23f6

muibot added the PR: Needs Review label Mar 28, 2017

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 29, 2017

oliviertassinari reviewed Mar 29, 2017

View reviewed changes

oliviertassinari approved these changes Mar 30, 2017

View reviewed changes

oliviertassinari added PR: Review Accepted and removed PR: Needs Review labels Mar 30, 2017

mbrookes reviewed Mar 30, 2017

View reviewed changes

mbrookes added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Review Accepted labels Mar 30, 2017

[DatePicker] update docs to reflect boolean change

ee28506

muibot added the PR: Needs Review label Mar 31, 2017

oliviertassinari removed the PR: Needs Review label Apr 3, 2017

[DatePicker] update prop description to end with period

948ceed

muibot added the PR: Needs Review label Apr 5, 2017

muibot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: Needs Review labels Apr 13, 2017

oliviertassinari closed this May 21, 2017

oliviertassinari mentioned this pull request Jun 24, 2017

[Discussion] 'viewYearFirst' prop for DatePicker component #7222

Closed

oliviertassinari mentioned this pull request Jul 7, 2017

[Datepicker] Option to select year first #7367

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DatePicker] allow year selecton to be open before month/day selection #6420

[DatePicker] allow year selecton to be open before month/day selection #6420

3upzorz commented Mar 23, 2017 •

edited

mbrookes commented Mar 24, 2017

oliviertassinari commented Mar 24, 2017

oliviertassinari Mar 24, 2017

oliviertassinari Mar 24, 2017

mbrookes commented Mar 24, 2017

3upzorz commented Mar 24, 2017

mbrookes commented Mar 24, 2017 •

edited

3upzorz commented Mar 24, 2017

oliviertassinari commented Mar 24, 2017 •

edited

oliviertassinari left a comment

3upzorz commented Mar 27, 2017

oliviertassinari commented Mar 27, 2017 •

edited

oliviertassinari commented Mar 27, 2017

oliviertassinari left a comment

oliviertassinari Mar 29, 2017

mbrookes Mar 30, 2017

3upzorz commented Mar 30, 2017

mbrookes Mar 30, 2017

mbrookes commented Mar 31, 2017 •

edited

3upzorz commented Apr 5, 2017

oliviertassinari commented Apr 5, 2017 •

edited

3upzorz commented Apr 6, 2017

oliviertassinari commented Apr 8, 2017 •

edited

oliviertassinari commented May 21, 2017

davidhantson commented Jun 12, 2017

oliviertassinari commented Jun 12, 2017 •

edited

[DatePicker] allow year selecton to be open before month/day selection #6420

[DatePicker] allow year selecton to be open before month/day selection #6420

Conversation

3upzorz commented Mar 23, 2017 • edited

mbrookes commented Mar 24, 2017

oliviertassinari commented Mar 24, 2017

oliviertassinari Mar 24, 2017

Choose a reason for hiding this comment

oliviertassinari Mar 24, 2017

Choose a reason for hiding this comment

mbrookes commented Mar 24, 2017

3upzorz commented Mar 24, 2017

mbrookes commented Mar 24, 2017 • edited

3upzorz commented Mar 24, 2017

oliviertassinari commented Mar 24, 2017 • edited

oliviertassinari left a comment

Choose a reason for hiding this comment

3upzorz commented Mar 27, 2017

oliviertassinari commented Mar 27, 2017 • edited

oliviertassinari commented Mar 27, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Mar 29, 2017

Choose a reason for hiding this comment

mbrookes Mar 30, 2017

Choose a reason for hiding this comment

3upzorz commented Mar 30, 2017

mbrookes Mar 30, 2017

Choose a reason for hiding this comment

mbrookes commented Mar 31, 2017 • edited

3upzorz commented Apr 5, 2017

oliviertassinari commented Apr 5, 2017 • edited

3upzorz commented Apr 6, 2017

oliviertassinari commented Apr 8, 2017 • edited

oliviertassinari commented May 21, 2017

davidhantson commented Jun 12, 2017

oliviertassinari commented Jun 12, 2017 • edited

3upzorz commented Mar 23, 2017 •

edited

mbrookes commented Mar 24, 2017 •

edited

oliviertassinari commented Mar 24, 2017 •

edited

oliviertassinari commented Mar 27, 2017 •

edited

mbrookes commented Mar 31, 2017 •

edited

oliviertassinari commented Apr 5, 2017 •

edited

oliviertassinari commented Apr 8, 2017 •

edited

oliviertassinari commented Jun 12, 2017 •

edited