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

[DatePicker] Add option to hide date display #6161

Merged
merged 3 commits into from
Feb 22, 2017

Conversation

dhoward
Copy link

@dhoward dhoward commented Feb 16, 2017

This adds an option to hide the date display above the date picker (or on the side on portrait mode). I did not add any unit tests because this is purely a visual change, but the code has been linted and docs have been added.

Thanks for the awesome library!

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

screen shot 2017-02-15 at 10 14 30 pm

@stunaz
Copy link
Contributor

stunaz commented Feb 16, 2017

By doing so, you lost the ability to choose the years

@dhoward
Copy link
Author

dhoward commented Feb 16, 2017

@stunaz yes that is true, sorry I forgot to mention that. I figured it would be ok since you already have a disableYearSelection option for users who do not need that functionality. I could add a line in the documentation something like “Hiding the Date Display will also hide year selection” to clarify that this will hide both. Does that sound ok?

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

This extra option sounds fair to me.

did not add any unit tests because this is purely a visual change

There is no reason why visual changes shouldn't be tested. I would even advocate for the opposite, they are the most tricky ones.
It's more a changing rendering logic here. I small unit test at the React level would be great.

@@ -42,6 +42,7 @@ class Calendar extends Component {
onTouchTapOk: PropTypes.func,
open: PropTypes.bool,
shouldDisableDate: PropTypes.func,
showDateDisplay: PropTypes.bool,
Copy link
Member

Choose a reason for hiding this comment

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

I have some consideration for the naming convention and the API consistency. With boolean properties, we have two options. showX/hideX I think that we should be choosing based on the default value. That's basically what have been done at the HTML specification, e.g. disabled, readonly with the input in order to allow the shorthand.
Let's follow the same convention. with hideX as the default is going to be false.

TL;DR What about hideCalendarDate?

@oliviertassinari oliviertassinari added component: date picker This is the name of the generic UI component, not the React module! new feature New feature or request 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 Feb 19, 2017
@mbrookes
Copy link
Member

By doing so, you lost the ability to choose the years

#4949 would solve that.

@dhoward
Copy link
Author

dhoward commented Feb 20, 2017

@oliviertassinari thanks for the feedback, I have renamed the prop and added unit tests for showing/hiding the date.

It looks like TravisCI is failing due to lint errors in a couple unrelated files, I see at least one of them is already being addressed in #6182, not sure about the other one but I can add a fix on this PR if that would be helpful.

@oliviertassinari oliviertassinari added PR: Review Accepted 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 Feb 21, 2017
@oliviertassinari oliviertassinari merged commit 4536604 into mui:master Feb 22, 2017
@oliviertassinari
Copy link
Member

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: date picker This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants