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

Provide a way to disable days #52

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

bseber
Copy link
Contributor

@bseber bseber commented Nov 19, 2020

  • provide a way to disable days via dateAdapter.isDateDisabled
    • optional function to avoid breaking changes

closes #15

@taromorimoto
Copy link

This is great! 🥳 Can we get this to 1.2 beta? 😄

@derTobsch
Copy link

This looks good to me! Are there any plans to add this?

@WickyNilliams
Copy link
Contributor

WickyNilliams commented Dec 8, 2020

I'll try to take a look at this later this week. I have a lot going on at the moment, so can't dedicate much time. On top of reviewing the code, I'll need to test how this behaves using keyboard and in various screen reader/browser combos. So it's not a quick merge unfortunately!

Thanks for making the PR @bseber

@WickyNilliams
Copy link
Contributor

I tested this today, and I immediately hit issues when navigating dates using the keyboard. The focus marker is not visible when you navigate to a disabled date, so it is not clear to the user what is happening. I don't have time to test this further today, I'll need do some more investigation another time

@bseber bseber force-pushed the provide-a-way-to-disable-days branch from 5f50bd1 to 071cc2f Compare January 28, 2021 19:06
@bseber
Copy link
Contributor Author

bseber commented Jan 28, 2021

The focus marker is not visible when you navigate to a disabled date, so it is not clear to the user what is happening.

🤔 That's a tricky one

we could highlight disabled days with box-shadow like:

duet-date-picker

  &:focus,
  &.is-disabled[tabindex="0"] {
    box-shadow: 0 0 5px var(--duet-color-primary);
    z-index: 200;
  }

To remove the focused style from the previously selected day, however, we have to blur it manually within the duet-date-picker.tsx handleKeyboardNavigation method.

    if (document.activeElement instanceof HTMLElement) {
      document.activeElement.blur()
    }

which in turn breaks the keyboard navigation since there is no focused element anymore. ArrowDown will scroll the page now.

handleKeyboardNavigation listens to the keyDown even of every button element. we could attach a global keyboard listener to the document and react to it when our date-picker-instance is focused currently (should be the activeFocus attribute of the DuetDatePicker)

what do you think about this idea?

@WickyNilliams
Copy link
Contributor

I was thinking of maybe using aria-disabled for those dates? Then they could still be focusable etc, but it would be presented as disabled to screen readers. I need to give it some more thought

@derTobsch
Copy link

Good morning @WickyNilliams, this would be a great feature for us. Can I help you to get this faster into a release?

@jrparish
Copy link

We're itching to get at this feature as well. Please let us know if there is anything we can do to help 😄

make `isDateDisabled` optional to not introduce a breaking change
enabling selection of every visible day in the date picker
should have no impact on the visualization.

days of next or previous month could be selectable
but should still be rendered differently to the focused
month.
@bseber bseber force-pushed the provide-a-way-to-disable-days branch from 071cc2f to 1000f33 Compare April 24, 2021 20:23
@bseber
Copy link
Contributor Author

bseber commented Apr 24, 2021

@WickyNilliams I rebased on master and added a commit to render disabled days as a span element. Feels good to me so far. I appreciate your feedback :-)

@WickyNilliams
Copy link
Contributor

@bseber thanks, this looks good at a glance! I will do some testing later this week. There are a few small changes I would suggest, though i'm happy to make them myself before publishing a new version, to save on some back and forth. Unless you'd like to do it! Happy to list my suggestions if so. Let me know.

@WickyNilliams
Copy link
Contributor

Also, forgot to say, I really appreciate the tests!

@bseber
Copy link
Contributor Author

bseber commented Apr 28, 2021

@bseber thanks, this looks good at a glance! I will do some testing later this week. There are a few small changes I would suggest, though i'm happy to make them myself before publishing a new version, to save on some back and forth. Unless you'd like to do it! Happy to list my suggestions if so. Let me know.

feel free to do the small changes yourself :-)

@WickyNilliams
Copy link
Contributor

WickyNilliams commented Apr 30, 2021

Initial testing seems like this works really well.

  • VoiceOver: "Sunday, 4th April, dimmed, toggle button, column 6 of 7"
  • JAWS: "4th April, toggle button, unavailable, column 7"
  • NVDA: "SUNDAY column 7 toggle button, unavailable, not pressed, April 4th"

I will now merge and make some changes myself. The changes i'm going to make:

  • move isDateDisabled off DateAdapter, since this functionality is unrelated to formatting/parsing dates - the isDateDisabled logic would remain the same for every/any date format. I will make it a top level option instead.
  • Attempt to use <button> with aria-disabled instead of <span> for disabled days. I think this will be more consistent, and should simplify the code somewhat

@WickyNilliams WickyNilliams merged commit 1ebd1a2 into duetds:master Apr 30, 2021
@WickyNilliams
Copy link
Contributor

WickyNilliams commented Apr 30, 2021

Also i want to simplify things so that isDateDisabled can look like this:

picker.isDateDisabled = function isWeekend(date) {
  return date.getDay() === 0 || date.getDay() === 6;
}

Rather than this:

picker.isDateDisabled = function isWeekendPlusSomeOtherLogicSadFace(date, focusedDay) {
  return (
    date.getDay() === 0 ||
    date.getDay() === 6 ||
    !(date.getFullYear() === focusedDay.getFullYear() && date.getMonth() === focusedDay.getMonth())
  )
}

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

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

Successfully merging this pull request may close these issues.

Provide a way to disable days per week or dates.
5 participants