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 isInvalidDay support #12962

Merged
merged 1 commit into from Jan 25, 2019

Conversation

Projects
None yet
5 participants
@psealock
Copy link
Contributor

psealock commented Dec 18, 2018

Description

Add the concept of unselectable days to the DatePicker's calendar by exposing react-dates prop isOutsideRange. Here is an example with Mondays made as invalid selections:

screen shot 2018-12-18 at 12 52 13 pm

What about invalidating dates via the inputs?

The changes here affect only the calendar (<DatePicker /> component) and not the inputs (<TimePicker />). Ideally, the same validation is passed along to the inputs to prevent selection of invalid days via the keyboard. That will take some design work to surface why an invalidation has occurred. I propose to handle that separately.

Testing instructions

Since the concept of invalid days has no meaning for the text editor, changing code temporarily is the only way to evaluate this change.

  1. Add this prop to invalidate Mondays
isInvalidDate={ ( date ) => 1 === date.getDay() }

to the implementation of <DatePicker />,

<DatePicker
currentDate={ currentDate }
onChange={ onChange }
/>

  1. Go to a new post /wp-admin/post-new.php#/.
  2. Open the date picker.
  3. See Mondays are not selectable via the calendar.
  4. Using VoiceOver, confirm focus of an invalid calendar date adds a "Not available" to the description.

Types of changes

New feature: Expose a prop, isInvalidDay , to <DatePicker />.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@designsimply designsimply added this to the 4.9 (Gutenberg) milestone Jan 17, 2019

@designsimply designsimply requested a review from WordPress/gutenberg-core Jan 17, 2019

@psealock psealock force-pushed the psealock:add/datepicker-isOutsideRange-support branch from 4269e8f to 912ca49 Jan 20, 2019

@timmyc
Copy link
Contributor

timmyc left a comment

Looking good to me, just one minor comment.

@@ -36,7 +36,7 @@ class DatePicker extends Component {
}

render() {
const { currentDate } = this.props;
const { currentDate, isInvalidDay } = this.props;

This comment has been minimized.

@timmyc

timmyc Jan 22, 2019

Contributor

Probably not totally required but should we provide a default prop for this of false just to be extra explicit?

This comment has been minimized.

@psealock

psealock Jan 22, 2019

Author Contributor

Thanks @timmyc !

I could set a default prop or just { isInvalidDay = false } = this.props, but I think its best to pass along an undefined value to react-dates's own default prop for isOutsideRange:

https://github.com/airbnb/react-dates/blob/d572bea9029739089bd200026be3aad7684a8b02/src/components/DayPickerSingleDateController.jsx#L100

The reason is because React handles an undefined prop differently than null or possibly false (https://reactjs.org/docs/react-component.html#defaultprops).

@psealock psealock force-pushed the psealock:add/datepicker-isOutsideRange-support branch from 912ca49 to caa76ba Jan 22, 2019

@timmyc

timmyc approved these changes Jan 23, 2019

Copy link
Contributor

timmyc left a comment

This is looking good to me.

@aduth
Copy link
Member

aduth left a comment

The DateTime component has a README.md . The prop should be documented.

@psealock psealock force-pushed the psealock:add/datepicker-isOutsideRange-support branch 2 times, most recently from 757ec8d to 33d6217 Jan 23, 2019

@psealock

This comment has been minimized.

Copy link
Contributor Author

psealock commented Jan 23, 2019

Thanks for the review @aduth. Changes made to DateTime Readme.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Jan 24, 2019

Moment is not otherwise exposed as part of the public interface, and I don't think we'd want to start now (related: #11418).

What would you think about passing the Date object instead?

https://momentjs.com/docs/#/displaying/as-javascript-date/

@psealock

This comment has been minimized.

Copy link
Contributor Author

psealock commented Jan 24, 2019

@aduth I've modified how isInvalidDay is passed on to react-dates such that the prop function's argument is a Date object instead of Moment.

I updated the test instructions on this PR. Thanks again for the input.

@@ -36,7 +36,7 @@ class DatePicker extends Component {
}

render() {
const { currentDate } = this.props;
const { currentDate, isInvalidDay } = this.props;

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

Sorry, I'm nitpicking at this point, but the term "Day" is ambiguous to me, where the value itself to check is a Date object. I'm also a bit wary about the negation here, regardless of what react-dates is doing, since it seems a bit unnecessary to reverse what is essentially a validity check.

Thoughts on isValidDate instead?

There's a further argument about the naming of the prop implying that its value is itself a boolean, when in fact it's a callback function. I'm not sure what precedent there is here for these sorts of callback functions. I'm not overly concerned about it.

This comment has been minimized.

@psealock

psealock Jan 24, 2019

Author Contributor

I'm also a bit wary about the negation here, regardless of what react-dates is doing, since it seems a bit unnecessary to reverse what is essentially a validity check.

One advantage of the negation relates to the optionality of the prop. By default, all dates are valid. It would seem odd to me that by adding a positively-framed check, isValidDate, I'd have to explicitly return true to enable a date that is otherwise enabled by default.

the term "Day" is ambiguous to me

Great point.

My slight preference is isInvalidDate, but I can go with the positive isValidDate as well.

This comment has been minimized.

@aduth

aduth Jan 24, 2019

Member

I'd be fine with isInvalidDate

This comment has been minimized.

@psealock

psealock Jan 24, 2019

Author Contributor

Cool, done and rebased.

@psealock psealock force-pushed the psealock:add/datepicker-isOutsideRange-support branch from 53e3aef to 006fdfc Jan 24, 2019

@aduth

aduth approved these changes Jan 25, 2019

Copy link
Member

aduth left a comment

Looks great 👍 Thanks for your patience.

@aduth aduth merged commit f7ade90 into WordPress:master Jan 25, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

daniloercoli added a commit that referenced this pull request Jan 26, 2019

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…rnmobile/372-enter-key-detection-to-title

* 'master' of https://github.com/WordPress/gutenberg: (29 commits)
  Update for RangeControl documentation (#12564)
  Plugin: Deprecate gutenberg_load_list_reusable_blocks (#13456)
  Update the columns attribute in onSelectImages so that if images are removed via the media modal, the columns can't be higher than the new number of images (#13488)
  Replace the fullscreen "exit" icon with a back arrow (#13403)
  Include :visited links in button color (#12183)
  Amazon Kindle block (#13510)
  Plugin: Deprecate gutenberg_prepare_blocks_for_js (#13457)
  Add watcher on Linux: change fs to node-watch (#13448)
  Plugin: Deprecate `gutenberg` theme support (#13458)
  Datepicker: Add inValidDay support (#12962)
  Block Switcher: Render disabled button even if multi-selection (#13431)
  Plugin: Deprecate gutenberg_register_post_types (#13468)
  Plugin: Deprecate register_tinymce_scripts (#13466)
  Set minimum of words for RSS excerpt (#13502)
  Plugin: Deprecate gutenberg_get_block_categories (#13454)
  Plugin: Deprecate gutenberg_content_block_version (#13469)
  API Fetch: Expose nonce on created middleware function (#13451)
  Plugin: Remove list screens integrations (#13459)
  Plugin: Remove core-defined block detection functions (#13467)
  Spec Parser: Move generated spec parser to package (#13493)
  ...

@psealock psealock deleted the psealock:add/datepicker-isOutsideRange-support branch Jan 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment