Skip to content

[DatePicker] Use DatePicker for ranged example min and max date selection#4010

Merged
oliviertassinari merged 1 commit intomui:masterfrom
aahan96:range_DatePicker
May 4, 2016
Merged

[DatePicker] Use DatePicker for ranged example min and max date selection#4010
oliviertassinari merged 1 commit intomui:masterfrom
aahan96:range_DatePicker

Conversation

@aahan96
Copy link
Copy Markdown
Contributor

@aahan96 aahan96 commented Apr 15, 2016

Now the ranged examples use DatePicker for minDate and maxDate.

Closes issue #4009

  • 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).

@aahan96 aahan96 force-pushed the range_DatePicker branch 2 times, most recently from 70d1db6 to 3754cd8 Compare April 15, 2016 21:44
@aahan96
Copy link
Copy Markdown
Contributor Author

aahan96 commented Apr 15, 2016

Now, we can use the DatePicker to select min and max dates.

screen shot 2016-04-15 at 4 45 41 pm

@owencm
Copy link
Copy Markdown
Contributor

owencm commented Apr 16, 2016

Note to reviewers that this change affects the ranged DatePicker example and replaces the two text inputs that specify the range itself and replace them with date pickers.

Personally I'd use the name event (assuming that's what it actually is) or _ in the place of x in handleChangeMinDate = (x, date).

Otherwise, assuming the linting passes this looks like a reasonable change to me.

@nathanmarks
Copy link
Copy Markdown
Contributor

@aahan96

@owencm 's suggestion to replace the x with event would be the idiomatic choice here 👍

@mbrookes
Copy link
Copy Markdown
Member

@aahan96 - please also follow the PR template.

import TextField from 'material-ui/TextField';
import Toggle from 'material-ui/Toggle';

const DateTimeFormat = global.Intl.DateTimeFormat;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be undefined on browser that don't support Intl.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aahan96
Copy link
Copy Markdown
Contributor Author

aahan96 commented Apr 19, 2016

@oliviertassinari Yeah, I just checked it and there was a mix match of both controlled and uncontrolled behaviors. So, for now, I have switched it toggled={this.state.autoOk}' as it has already been set tofalse` in the upper part of file.

@mbrookes @owencm @nathanmarks I have changed x to event as it makes more sense. I used x as it wasn't being used, but later realized that it can cause confusion and doesn't follow the code.

@aahan96 aahan96 force-pushed the range_DatePicker branch from c24b583 to 7e24808 Compare May 4, 2016 15:11
@aahan96
Copy link
Copy Markdown
Contributor Author

aahan96 commented May 4, 2016

@oliviertassinari Just wanted to check with the status of this pr. I have also added support for browsers that don't support the IntlLocales. Let me know if something needs to be changed 😃

@oliviertassinari
Copy link
Copy Markdown
Member

@aahan96 That looks good from a code point of view. I will try it out 👍.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation. label May 4, 2016
@mbrookes
Copy link
Copy Markdown
Member

mbrookes commented May 4, 2016

@aahan96 I appreciate you've gone to the trouble of making the displayed date look the same as the manual textfield that this replaces, but as this isn't a localisation example, I think the default ISO 8601 date is fine, and make the example code much simpler. 👍

@aahan96 aahan96 force-pushed the range_DatePicker branch from 7e24808 to b3d21b1 Compare May 4, 2016 19:39
@aahan96
Copy link
Copy Markdown
Contributor Author

aahan96 commented May 4, 2016

@mbrookes I removed the localization example and the dates are now displayed in default ISO 8601 format. Thanks for the advice 👍

@mbrookes
Copy link
Copy Markdown
Member

mbrookes commented May 4, 2016

@aahan96 Thanks for doing that.

Please could you also remove the default date from the first DatePicker? It makes it easier to see which one is the actual demo picker.

I can't decide whether the range-setting pickers should honor the toggles, but I guess it's harmless. 😄

@aahan96 aahan96 force-pushed the range_DatePicker branch from b3d21b1 to bf3f555 Compare May 4, 2016 21:05
@aahan96
Copy link
Copy Markdown
Contributor Author

aahan96 commented May 4, 2016

@mbrookes Done!

@mbrookes mbrookes changed the title [DatePicker][Docs]Range date picker [DatePicker] Use DatePicker for ranged example min and max date selection May 4, 2016
@mbrookes
Copy link
Copy Markdown
Member

mbrookes commented May 4, 2016

Thanks!

Please go ahead and squash, and 'reword' the base commit title to match the PR title, and I think we can merge. 👍

…tion

Fixed some issues with the pr

Removed default date
@oliviertassinari oliviertassinari merged commit 3c93c9c into mui:master May 4, 2016
@oliviertassinari
Copy link
Copy Markdown
Member

@aahan96 Thanks, that feel better now!
I'm wondering if 2 years between the min and the max isn't too much. E.g. I had to set 10 days to make sure it was working correctly.

@aahan96 aahan96 deleted the range_DatePicker branch August 11, 2016 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Improvements or additions to the documentation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants