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

Clean up prop types definitions for top-level components #332

Merged
merged 1 commit into from
Feb 17, 2017

Conversation

majapw
Copy link
Collaborator

@majapw majapw commented Feb 16, 2017

This change does the following

  • Make onDate(s)Change and onFocusChange required
  • Sort DateRangePicker, SingleDatePicker, DayPicker by category with comments
  • Use forbidExtraProps on all components
  • Add propTypes and defaultProps to the examples so that information shows up in the info addon

I would like ideally to have focusedInput/startDate/endDate and focused/date to be required as well, but since those can take a value of null that's a bit trickier. I might make a validator that allows for this (something like isDefined).

to: @airbnb/webinfra

@majapw majapw added the semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep. label Feb 16, 2017
import { START_DATE, END_DATE, HORIZONTAL_ORIENTATION, ANCHOR_LEFT } from '../constants';
import isInclusivelyAfterDay from '../src/utils/isInclusivelyAfterDay';

const propTypes = omit(DateRangePickerShape, [
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use object rest here instead of omit and avoid the dependency,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a dev dependency so I didn't think it was a big deal. Doesn't using object rest require unused vars to be ignored?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep

- Make `onDate(s)Change` and `onFocusChange` required
- Sort `DateRangePicker`, `SingleDatePicker`, `DayPicker` by category with comments
- Use `forbidExtraProps` on all components
- Add propTypes and defaultProps to the examples so that information shows up in the info addon
@majapw majapw force-pushed the maja-make-required-props-actually-required branch from da33d60 to 064c5af Compare February 16, 2017 23:07
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 86.839% when pulling 064c5af on maja-make-required-props-actually-required into 3e534c8 on master.

@ljharb
Copy link
Member

ljharb commented Feb 17, 2017

airbnb-prop-types now contains explicitNull, if that's useful here?

@majapw
Copy link
Collaborator Author

majapw commented Feb 17, 2017

Sweet. I'll add that in. :)

@majapw majapw merged commit 40ca637 into master Feb 17, 2017
@majapw majapw deleted the maja-make-required-props-actually-required branch February 17, 2017 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major: breaking change A non-backwards-compatible change; anything that breaks code - including adding a peerDep.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants