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

added sl-date-picker component #486

Closed
wants to merge 8 commits into from

Conversation

hanc2006
Copy link

New sl-date-picker component to shoelace library. This PR replaces the old one #474

@vercel
Copy link

vercel bot commented Jul 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/shoelace/shoelace/3SB5843W9c3jFyy9bybLdS9wAzYV
✅ Preview: https://shoelace-git-fork-hanc2006-sl-date-picker-shoelace.vercel.app

@claviska
Copy link
Member

Thanks for the fresh PR. This isn't an easy component and you've done a fantastic job on it! Please don't take this constructive criticism the wrong way.

Some things I noticed when testing it out:

  • There should probably be a way for mouse/touch users to scroll up/down when selecting a year from this view:
    CleanShot 2021-07-23 at 19 26 12@2x
  • It's weird that when pressing up/down into a new "page" the year is activated, but when pressing arrows on the same "page" the year stays the same and instead is just "focused".
  • When tabbing into the control with tab and shift+tab, it appears to capture tabbing so you can't tab out of the date picker.
  • When pressing left/right in this view, the cursor wraps to the next row, but when pressing up/down, it wraps to the same column. Row wrapping should probably work like columns, so instead of Jan => Feb => March => April it goes Jan => Feb => March => Jan.
    CleanShot 2021-07-23 at 19 28 54@2x

Additionally, I'm seeing "Calendar" components and I think they make a lot of sense for various applications. Would it be easy to split this into <sl-calendar> and <sl-date-picker>?

The benefits being:

  • More easily focus on calendar logic and testing with <sl-calendar>
  • More easily determine if new features make sense to add to the calendar
  • It becomes easier to implement <sl-date-picker> with <sl-calendar> as a dependency
  • Additional components that require a calendar will have a common dependency

In fact, we could focus on making <sl-calendar> rock solid, then tie that implementation into either <sl-date-picker> or possibly even <sl-input type="date"> (idk, maybe).

I'm interested to hear your opinion!

@claviska
Copy link
Member

Also just noticed that tabbing through in the month and year views loops through the header, so the months/years aren't keyboard accessible without focusing on them first with the mouse.

@hanc2006
Copy link
Author

hanc2006 commented Jul 25, 2021

Please don't take this constructive criticism the wrong way.

Why? If criticism is constructive, and it is useful to improve my work, it is well accepted. Otherwise, what is the purpose to share the code with other people? 😄

There should probably be a way for mouse/touch users to scroll up/down when selecting a year from this view.

I'm working on this. I want to create a new internal helper class, to use when you want to capture mouse/touch events.

When tabbing into the control with tab and shift+tab, it appears to capture tabbing so you can't tab out of the date picker.

Yes, it is currently a loop. How should the navigation priority be set with the tab key?

The path described above is used in the duet calendar. Instead, for the online calendar?

It's weird that when pressing up/down into a new "page" the year is activated, but when pressing arrows on the same "page" the year stays the same and instead is just "focused".

For this issue, I have to disable the automatic month selection when I re-render the view, and it should be fixed.

When pressing left/right in this view, the cursor wraps to the next row, but when pressing up/down, it wraps to the same column. Row wrapping should probably work like columns, so instead of Jan => Feb => March => April it goes Jan => Feb => March => Jan.

Vertical navigation between months should be changed. Your proposal makes more sense.

Additionally, I'm seeing "Calendar" components and I think they make a lot of sense for various applications. Would it be easy to split this into and ?

Currently, the "Calendar" class offer simple methods for working with dates, and can generate
an array of days in a given month. What exactly should the <sl-calendar> component do?

  • Create an internal utility class to handle click, tap, double tap and swipe events inside components.
  • Improve keyboard navigation (tab)
  • Disable automatic month selection
  • Change vertical navigation between months

@claviska
Copy link
Member

Please don't take this constructive criticism the wrong way.

Why? If criticism is constructive, and it is useful to improve my work, it is well accepted. Otherwise, what is the purpose to share the code with other people? 😄

Because you're doing great work and putting a lot of time and effort into contributing! I don't want to discourage you from that or undervalue your effort. You've been tackling some of the much more difficult components/APIs and with that territory comes a lot of little things, nitpicks, details, and polish that make it difficult to get to the finish line.

I can't help but feel a bit of guilt because my standard for quality is very high in this project. I'd rather overthink these things and build them out multiple times to make sure we get it right rather than rush it in. Longevity is a key objective of Shoelace, and some may perceive this as a high barrier of entry for contribution.

How should the navigation priority be set with the tab key?

  • Tabbing through the header should be natural tab order
  • Once you hit the day/month/year views, those should be treated as a single tab stop (arrow keys to navigate, tab/shift+tab to leave)
  • Tabbing on the last tabbable element within the date picker should close it and allow the next tabbable element in the document to receive focus
  • Shift + tabbing on the first tabbable element within the date picker should close it and move focus to the date picker button

What exactly should the component do?

This would render the calendar inline, giving users the ability to show a read only calendar or a potentially interactive one. Thinking further ahead, it might be possible to allow the user to display events on certain days. There could be a lot of overlap in features, so the thought of breaking it out came up. It would be easier to test as well.

@hanc2006
Copy link
Author

hanc2006 commented Aug 28, 2021

Hi @claviska,

these latest commits are a complete rework of the sl-date-picker component. I have simplified and optimized several parts of the code from the first committed version.

Additionally, I'm seeing "Calendar" components and I think they make a lot of sense for various applications. Would it be easy to split this into sl-calendar and sl-date-picker?

I have split the code into two components as suggested. The code is now easier to manage.

In fact, we could focus on making sl-calendar rock solid, then tie that implementation into either sl-date-picker.

I focused my work on this part, to create a small and reusable calendar library.

What is missing from these latest commits:

  • tab key navigation
  • improve the appearance of the native html select (used to select calendar months)
  • use more custom cssparts
  • dark theme

@mcjazzyfunky
Copy link

mcjazzyfunky commented Aug 29, 2021

@hanc2006 @claviska Just as a side note:

In fact, we could focus on making rock solid

As it is a hell of work (implementing and reviewing and discussing etc) to finalize this, an option could be to split work a bit and to use a 3rd party "calendar sheet" in <sl-calendar> in the beginning (for Shoelace v2.0) and replace that part (if desired) later when there's more time. So we have more energy to focus on the real important stuff (what date related custom elements are necessary, how to handle i18n properly -> see #419, how to provide good UX etc.).

Here's a little Demo using vanillajs-datepicker:
» DEMO

In file sl-calendar.ts you find a skeleton of the calendar component (it uses this I18n object that I currently use in all my localizable demos - just ignore it if you want ... the important part is that the localization logic in that demo is based on Intl, while normally vanillajs-datepicker does not use Intl).
In file sl-calendar.styles.ts you find some CSS adaptions for shoelace ... I have not changed all colors to SL colors yet, customizing all colors and making it also look fine in dark mode will take some hours, I guess.
The locale can be modified in the document's html element (see index.html).

Basically, something that I have already suggested a couple of month ago in #101. There has not been any response then, so maybe such a solution is not wanted in the first place ... anyway ...


[Edit] Important: The demo uses that vanillajs-datepicker in so-called "inline mode". Unfortunately, vanillajs-datepicker does not support keyboard navigation in inline mode. So, I guess, this is a showstopper for temporary use of this library in Shoelace.

@hanc2006
Copy link
Author

@mcjazzyfunky,

Shoelace is a web component library. Using a third-party library to replace a web component doesn't make much sense. It would make more sense to use a third-party library like moment.js or something similar to support a component behavior.

For i18n localization support, I think it takes some time to understand which of the proposed solutions is best to use (or maybe use a new one).

@mcjazzyfunky
Copy link

mcjazzyfunky commented Aug 29, 2021

Shoelace is a web component library. Using a third-party library to replace a web component doesn't make much sense.

Of course it makes complete sense. This sl-calendar in the demo is a 100% normal custom element, there's nothing special about it. You cannot see from outside (without looking at code) whether sl-calendar has used a third-party library internally or not. You can always use vanillajs stuff in custom elements if you want, why not? (SSR might be an issue, but SSR is always an issue with custom elements).

It would make more sense to use a third-party library like moment.js or something similar to support a component behavior.

This demo is mainly about "view" not about I18n ... the only I18n relevant is that the demo shows that vanillajs-datepicker allows to adapt its internal i18n functionality (things like that are not taken for granted).

It was just a proposal to save some time. If there's enough time for implementing and reviewing 100% self-written full-fledged date components before the v2.0 release, fine 😉....


[Edit - as also appended to the comment above] Important: The demo uses that vanillajs-datepicker in so-called "inline mode". Unfortunately, vanillajs-datepicker does not support keyboard navigation in inline mode. So, I guess, this is a showstopper for temporary use of this library in Shoelace.

@claviska
Copy link
Member

I’ll look at this tomorrow. Thanks for your continued effort and support, @hanc2006.

As for i18n, let’s not get side tracked here and continue tracking that in #419.

@claviska
Copy link
Member

dark theme

This branch is stale. If you merge next into it you'll get the latest and dark mode for free. Note the changelogs for beta.48 and beta.49 before merging, though.

tab key navigation

It looks like the calendar doesn't have any keyboard nav unless it's inside a date picker. Is this intentional?

@hanc2006
Copy link
Author

It looks like the calendar doesn't have any keyboard nav unless it's inside a date picker. Is this intentional?

It's a task I have to complete but I need more time.

Any other remarks? This feature sl-format-date integration with date library that I added what do you think?

@claviska
Copy link
Member

I'm going to close this PR because it's very stale now and there are a number of known issues.

I've been experimenting with a Calendar component in the calendar branch that can be used to compose a date picker. I'm also aware of at least two other open source, Lit-based date pickers that are in the making, both of which could be candidates for forking.

@claviska claviska closed this Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants