-
Notifications
You must be signed in to change notification settings - Fork 68
tweaks to disable date functionality to slightly simplify things #80
Conversation
}} | ||
tabIndex={isFocused ? 0 : -1} | ||
onClick={handleClick} | ||
onKeyDown={onKeyboardNavigation} | ||
disabled={isOutsideRange} | ||
aria-disabled={disabled ? "true" : undefined} | ||
disabled={isOutsideRange || !isMonth} |
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.
Working with the focused day and handling current year/month stuff should not be the developer's concern, this logic will always be required, so the picker should handle it :)
my use case is to get rid of the isMonth
check, actually :-)
we're using the date picker in an absence management tool and days are coloured differently when it is a vacation day or a sick day for example. Currently we have no visual difference between the last day of the month and the first day of the next month. And I prefer to keep it like that :-) The user sees the days, so why should the tool prohibit selecting it?
for a lib I think it is ok to say "you have to handle everything yourself, if you are overriding the default behaviour." The date picker has a nice simple default of just picking dates, and can be fully enhanced by power users.
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.
Oh do you mean as it behaves here? https://www.duetds.com/components/date-picker/
You can select any dates that show from the previous/next months. If so I simply forgot to to port that behaviour over and I'll make that change :)
@bseber just had a thought: this only prevents people selecting a date in the calendar, but they are still free to type in a disallowed date. Curious how you think this should be handled. I'm leaning towards it being the application developer's responsibility to check the date is allowed on change. You can even use the same function: <duet-date-picker></duet-date-picker>
<script>
const picker = document.querySelector("duet-date-picker")
function isWeekend(date) {
//...
}
picker.isDateDisabled = isWeekend
picker.addEventListener("duetChange", function(e) {
if (isWeekend(e.detail.valueAsDate)) {
alert("weekends not allowed!")
} else {
alert(`valid date: ${e.detail.value}`)
}
})
</script> Forgive any errors, I typed this up on mobile :) |
good catch. those edge cases every time 🙈 maybe we can take advantage of the constraint validation api?
input.addEventListener("invalid", function (event) {
//
});
input.checkValidity(); // trigger "invalid"
input.reportValidity(); // show native feedback
input.setCustomValidity("custom error message"); // mark as invalid
input.setCustomValidity(""); // mark as valid I will have a look at this the next days. Never had a chance to to this with custom elements :) |
As it happens, I have already implemented part of the constraint validation API on the internal version of the picker. Specifically I added the I will port this behaviour over eventually. But I think for now it is fine to just recommend devs handle the edge case above. You should always validate your data separately anyway so I don't think this is asking much/anything extra of devs. We can make a note of it in the docs so that it's clear |
By which I mean we can look into this more, but I'm happy to just ship this for now and incrementally improve on it later |
that would be awesome 😻
yes, one thing after another. thanks @WickyNilliams |
@bseber check out the latest changes on this PR.
Let me know if it all looks good to you |
@WickyNilliams looks good to me and works like a charm. Furthermore I've learned that |
84460c9
to
97d22b6
Compare
Finally released in |
@bseber here are my changes. They seem to work the same in my testing. Does anything stand out to you as wrong, or are there any parts you disagree with? Let me know :)