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

tweaks to disable date functionality to slightly simplify things #80

Merged
merged 5 commits into from Jun 18, 2021

Conversation

WickyNilliams
Copy link
Contributor

@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 :)

}}
tabIndex={isFocused ? 0 : -1}
onClick={handleClick}
onKeyDown={onKeyboardNavigation}
disabled={isOutsideRange}
aria-disabled={disabled ? "true" : undefined}
disabled={isOutsideRange || !isMonth}
Copy link
Contributor

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.

Copy link
Contributor Author

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 :)

@WickyNilliams
Copy link
Contributor Author

WickyNilliams commented May 3, 2021

@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 :)

@bseber
Copy link
Contributor

bseber commented May 3, 2021

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 :)

@WickyNilliams
Copy link
Contributor Author

As it happens, I have already implemented part of the constraint validation API on the internal version of the picker. Specifically I added the validity prop which implements ValidityState. I didn't do the rest yet as that was all that was needed for our use case. But it's something I'm keen on exploring more . See the example "date picker with validation error messages" here https://www.duetds.com/components/date-picker/ it's nice because it even respects min and max :)

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

@WickyNilliams
Copy link
Contributor Author

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

@bseber
Copy link
Contributor

bseber commented May 4, 2021

I will port this behaviour over eventually.

that would be awesome 😻

But I think for now it is fine to just recommend devs handle the edge case above. (...) 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

yes, one thing after another. thanks @WickyNilliams

@WickyNilliams
Copy link
Contributor Author

@bseber check out the latest changes on this PR.

  • You can now click dates outside of the month.
  • I slightly improved focus management for disallowed dates
  • Updated examples to show handling of cases where user manually inputs a disallowed date

Let me know if it all looks good to you

@bseber
Copy link
Contributor

bseber commented May 15, 2021

@WickyNilliams looks good to me and works like a charm.

Furthermore I've learned that disabled and aria-disabled can indeed be used in combination.

@WickyNilliams WickyNilliams force-pushed the feature/tweaks-to-disabled-days branch from 84460c9 to 97d22b6 Compare June 18, 2021 11:20
@WickyNilliams WickyNilliams merged commit 391d399 into master Jun 18, 2021
@WickyNilliams WickyNilliams deleted the feature/tweaks-to-disabled-days branch June 18, 2021 11:27
@WickyNilliams
Copy link
Contributor Author

Finally released in v1.4.0 🎉

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

Successfully merging this pull request may close these issues.

None yet

2 participants