-
-
Notifications
You must be signed in to change notification settings - Fork 146
Conversation
@tcbegley this looks like a really nice feature! Before looking at the code, the only thing I'd like to see is a plan for how we would extend this to support patterns of dates to disable, so that you don't have to enumerate them all:
We don't need to implement such patterns in this PR, I just want to make sure we have a consensus on how we would do that in the future, to ensure it wouldn't be a breaking change to the feature as implemented here. Do they fit into the same array with different values? A separate prop? |
That sounds like a good idea! Here's some initial thoughts, but note that I haven't really thought about implementation yet. Since the prop accepts date strings of the form
Days of the week seems harder and would maybe need a different prop. Perhaps it's as simple as Anyway, certainly interested to hear alternative suggestions. |
OK, having looked at the implementation (which is great, nice and clean! 🎉 ), I think it'll be fine to include arbitrary patterns within the same So I think all we really need is a test or two - perhaps one test for each component that a disabled date can't be chosen, plus a percy snapshot showing the open datepicker so we can see that the correct dates are visibly disabled. |
Actually we already build in Sunday=0 via |
Great! I'll add some tests in the next few days. |
Added some tests. From what I can tell by looking at the logs they are failing because CircleCI is installing dash-core-components 1.16.0, but when I build from this branch it builds version 1.15.0. Not sure the best way to fix that? |
Right, this is a quirk of our CI setup - we've released a new dash version since you created this PR, if you merge in the latest |
This is ready for review now, Percy is failing because I added new snapshots, but all seems to be working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent - nice tests! 💃
Hi,
I've seen a few people requesting on the community forum and elsewhere (e.g. here) that they have the option to make certain days unselectable in the day picker components beyond specifying a range with
{min,max}_date_allowed
.As it happens I previously implemented this for a colleague who had the same problem, so thought I would submit it here in case it's of interest. I'm not deeply invested in any aspect of this so if you don't like the interface / implementation / idea that's totally fine 😅 .
I think the main problem with this implementation is that if you wanted to for example disable all weekends over the span of several years you end up passing a pretty long list which could start to affect performance. I suspect for most applications it would be pretty hard to hit this limit but I haven't really investigated this much.
Here's a simple example you can use to test it. It makes only weekdays in April selectable.
which looks something like this