Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Calendar: Use isInvalidDate to disallow date selections #1685

Merged
merged 1 commit into from
Apr 9, 2019

Conversation

psealock
Copy link
Collaborator

@psealock psealock commented Feb 24, 2019

Introduce isInvalidDate to DatePicker and DateRange now that Gutenberg allows invalid days on the single date picker.

In an effort to make the two APIs identical, I removed the { invalidDays="future" } helper prop/function because it just masks functionality by adding complexity. I'd much rather expose the underlying ability to suit everyone's needs.

This is a breaking change, but we are still on the first version so I think its ok not to bump to 2.0.0. I'm open to bumping though.

Screenshots

screen shot 2019-02-25 at 12 37 28 pm

Detailed test instructions:

  1. Use master branch of Gutenberg or 7.1.0 of the components package.
  2. Customers Report.
  3. Show > Advanced Filters > Registered.
  4. See that future dates in the date picker are invalid and can't be selected.
  5. Orders Report > Date Range > Custom.
  6. Again, future dates should be invalidated.

Copy link
Collaborator

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

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

Use master branch of Gutenberg or 7.1.0 of the components package.

This is testing well for me with Gutenberg master. However, we don't currently require the Gutenberg plugin itself anymore for using wc-admin -- so should this hold off until 7.1.0 is released, so we can bump the version in package.json? Otherwise this change won't work for most.

I also have one optional comment about the date checks, and I do think the 2.0.0 version needs bumped.

@@ -1,3 +1,7 @@
# 1.7.0 (Unreleased)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a breaking change, but we are still on the first version so I think its ok not to bump to 2.0.0. I'm open to bumping though.

I think we actually do want to release a 2.0.0. The npm modules are using semver and we've previously published the other versions. We have also already done a 2.0.0 on the navigation package: https://github.com/woocommerce/wc-admin/blob/master/packages/navigation/CHANGELOG.md.

@@ -91,7 +92,7 @@ class DatePickerContent extends Component {
after={ after }
before={ before }
onUpdate={ onUpdate }
invalidDays="future"
isInvalidDate={ dateString => moment().isBefore( moment( dateString ), 'day' ) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Optional: What would you think about abstracting these one liners out to functions, so the name can make them a bit clearer (i.e. this one would be futureDaysInvalid or something)? I liked that 'future' previously implied that any future days were invalid. Now it takes a second to parse what's going on. This one is also used twice.

@psealock
Copy link
Collaborator Author

Thanks for the review @justinshreve !

Good call on 2.0.0. I went ahead and changed the version in components/package.json and if this is good to go, I can add the blocked label and we can wait until 7.1.0 of Gutenberg components is released.

Great suggestion too. I renamed a method isFutureDate for easier comprehension and added the same one in the other class to avoid inline functions as a prop.

Copy link
Collaborator

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

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

Thanks for the updates. Looks good!

I can add the blocked label and we can wait until 7.1.0 of Gutenberg components is released.

Sounds good to me. I think this is ready to go pending the release. One thing to note is I think we have some changes for a 1.7.0 of @woocommerce/components in an open PR, so you might have some rebasing to do later.

@justinshreve justinshreve added status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. and removed [Status] Needs Review labels Feb 28, 2019
@psealock
Copy link
Collaborator Author

psealock commented Mar 7, 2019

@ryelle
Copy link
Member

ryelle commented Apr 9, 2019

However, we don't currently require the Gutenberg plugin itself anymore for using wc-admin -- so should this hold off until 7.1.0 is released, so we can bump the version in package.json? Otherwise this change won't work for most.

The version in package.json is only used for tests, in the built file this is transpiled to wp.components so that we use the core-bundled version (or the gutenberg-bundled version). It looks like only @wordpress/[email protected] is included in WP 5.1.1, and @wordpress/[email protected] will be included in WP 5.2 when it's released (looks like April 30). Maybe we should require Gutenberg if we're going to rely on updates to GB?

In any case, I wanted to release @woocommerce/[email protected] this week, and was wondering if this should make it in (since it's now another major release). What do you think, @psealock ?

@psealock
Copy link
Collaborator Author

psealock commented Apr 9, 2019

In any case, I wanted to release @woocommerce/[email protected] this week, and was wondering if this should make it in (since it's now another major release). What do you think, @psealock ?

Thanks for looking into this one @ryelle. I think it should make it in because while the prop this PR makes use of, isInvalidDate, doesn't exist in @wordpress/[email protected] no changes will be seen until 7.2.2 is included. The functions passed down will simply be ignored by 7.0.8.

@psealock psealock removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Apr 9, 2019
@psealock
Copy link
Collaborator Author

psealock commented Apr 9, 2019

Since the repo was renamed, PRs created before the rename fail like this one is. Since they were passing before I rebased, I'm going to go ahead and merge this one.

@psealock psealock merged commit ea1caac into master Apr 9, 2019
@psealock psealock deleted the update/isInvalidDate branch April 9, 2019 21:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants