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

sl-datepicker component #474

Closed
wants to merge 18 commits into from
Closed

sl-datepicker component #474

wants to merge 18 commits into from

Conversation

hanc2006
Copy link

@hanc2006 hanc2006 commented Jun 30, 2021

Added the sl-datepicker component to shoelace library. This pull request don't include the component doc. Fixed timezone issue with date #467 (comment)

Other relevant commits:

  1. package.json added node engine constraint to support optional chaining operator (node >= 14)
  2. type safe emitter

@vercel
Copy link

vercel bot commented Jun 30, 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/8JGFHnzo6BEXfqYf5mBGn5oNtVp2
✅ Preview: https://shoelace-git-fork-hanc2006-next-shoelace.vercel.app

@mcjazzyfunky
Copy link

mcjazzyfunky commented Jul 1, 2021

Great!
Hope it's okay, that I give my 2 cents here.

  • I guess, as it is sl-color-picker, SlColorPicker, color-picker, color-picker.scss etc.
    it should accordingly be sl-date-picker, SlDatePicker, date-picker, date-picker.scss etc.

  • The icons (=> used via sl-icon) should use the system icon library not the default one.
    You can add those new system icons here.

  • [EDIT]: This item has been collapsed as it turned out to be not important for the further task/discussion - see the next comment for the reasons.
    The biggest challenge by far will be to find a simple but flexible way to make internationalization work properly in Shoelace (see RFC: i18n #419). The date picker will be the first SL component which really needs a very good answer to RFC: i18n #419 to do things right. For example it is currently not possible for the date picker to change the strategy, how the date localization is working. If I write a large app and I want to use i18n library `xyz`, how can I make the date picker component (or SL in general) using this `xyz` library for date localization? For example, I guess it's currently not possible that I use the date picker with locale `de-DE` but using the international date format `yyyy-mm-dd` for formatting (which is an often recommended date format for Germany, but not the commonly used one). Also for example, I am of the opinion that there should not be a date picker property `firstDayOfWeek` as this should already be determined by the locale. I know that it's currently not possible with the built-in JavaScript API to determine the first day of week by locale, but there's a good chance that it will be somewhere in future and in the meantime one workaround could be to use internally a `country-to-firstDayOfWeek` map for those countries where Monday is not the first day of week (this map may not be too big, I guess, please see [here](https://github.com/unicode-cldr/cldr-core/blob/master/supplemental/weekData.json#L59)). But as mentioned above, this is something that must be answered in RFC: i18n #419 first.

@claviska
Copy link
Member

claviska commented Jul 1, 2021

The biggest challenge by far will be to find a simple but flexible way to make internationalization work properly in Shoelace

All good points, especially this one. Shoelace is still at an early stage and there are some areas that need to thought about more in depth before jumping in too deep.

That said, I don't want things like this to stifle new components, especially contributions. For the time being, I'd suggest using a single prop for i18n as a stopgap:

myElement.i18n = {
  someTerm: 'Some Term',
  ...
};

This isn't intended to be a longterm i18n strategy and it won't handle pluralization, numerals, etc. but it will let contributors move forward with great ideas in a way that won't be hard to upgrade their work once we do solve i18n.

@mcjazzyfunky
Copy link

mcjazzyfunky commented Jul 1, 2021

Not really sure whether this is interesting, but just for information, here's how this is done in Vaadin web components:
https://vaadin.com/components/vaadin-date-picker/html-api/elements/Vaadin.DatePickerElement#property-i18n

@claviska
Copy link
Member

claviska commented Jul 1, 2021

@hanc2006
Copy link
Author

hanc2006 commented Jul 1, 2021

All good points, especially this one. Shoelace is still at an early stage and there are some areas that need to thought about more in depth before jumping in too deep.

That said, I don't want things like this to stifle new components, especially contributions. For the time being, I'd suggest using a single prop for i18n as a stopgap:

myElement.i18n = {
  someTerm: 'Some Term',
  ...
};

This isn't intended to be a longterm i18n strategy and it won't handle pluralization, numerals, etc. but it will let contributors move forward with great ideas in a way that won't be hard to upgrade their work once we do solve i18n.

#419 (comment)

@claviska
Copy link
Member

claviska commented Jul 8, 2021

Thanks for posting the docs so it's easier to play with. This is looking really good :)

A couple things I've noticed:

  • prev/next months are not keyboard navigable
  • arrow keys don't work as expected when moving between days (e.g. left and right skip days, up and down go too far)
  • selecting a date range in the Dropdown example hangs the browser (repro in Chrome: click calendar button, click 7, click 18)
  • there's easy way to select arbitrary months and, more importantly, years
  • it looks like the CSS custom properties are marked up with the old syntax which is why they're not showing in the docs
  • the calendar is rather big by default

This is one of the nicest looking calendar views I've seen — well done on the UI! 🤩

@claviska
Copy link
Member

claviska commented Jul 8, 2021

From this comment:

"It might be worth looking into how Duet and similar components have solved this for inspiration. 😆 😆 😆"
This function comes from there. Then I'll have to report a bug 🐛

Is this all original code? If we're using code from another project, we'll need to attribute them and include the necessary license(s).

@hanc2006
Copy link
Author

hanc2006 commented Jul 8, 2021

* rev/next months are not keyboard navigable

Browsing between months with Page Up, Page Down. Does not work?

  • Space, Enter: Selects a date, closes the dialog.
  • Arrow up: Moves focus to the same day of the previous week.
  • Arrow down: Moves focus to the same day of the next week.
  • Arrow right: Moves focus to the next day.
  • Arrow left: Moves focus to the previous day.
  • Home: Moves focus to the first day of the current week.
  • End: Moves focus to the last day of the current week.
  • Page Up: Changes to the previous month and sets focus on the same day of the same week.
  • Page Down: Changes to the next month and sets focus on the same day of the same week.

arrow keys don't work as expected when moving between days (e.g. left and right skip days, up and down go too far)

If you jump to a day of the month before/next the calendar switch month automatically. Do you mean this? This behavior can be changed.

there's easy way to select arbitrary months and, more importantly, years

It is not possible to select a year at the moment. Would you like to integrate a dropdown?

the calendar is rather big by default

Ok, i will change the default values ​​for the cell sizes. Is the calendar header also too big?

selecting a date range in the Dropdown example hangs the browser (repro in Chrome: click calendar button, click 7, click 18)

I tried with Brave in linux but I can't reproduce this issue 😢 .

it looks like the CSS custom properties are marked up with the old syntax which is why they're not showing in the docs

I will update the syntax with the new version

@hanc2006
Copy link
Author

hanc2006 commented Jul 8, 2021

Is this all original code? If we're using code from another project, we'll need to attribute them and include the necessary license(s).

No I only took a couple of functions, but with the new commit
#467 (comment) I replaced them 👍

@mcjazzyfunky
Copy link

mcjazzyfunky commented Jul 9, 2021

[Edit] I've just noticed there's already an issue regarding this topic: #191

@hanc2006 @claviska The support for RTL (right-to-left) is not proper for some shoelace components. The date picker also does not support RTL at the moment. For the date picker, RTL support might be a bit more challenging than for less complex components (for example that left-to-right arrow icon must be replaced with a right-to-left arrow icon, I guess), so maybe it would be an idea to start "fully RTL support" with the date picker to gain some experiences that might help to make the other SL components also fully RTL capable. What do you think?

@claviska
Copy link
Member

claviska commented Jul 9, 2021

  • Home/end work as described
  • Page up/down works somewhat as described. If you're on day 31, page up jumps to day 1 of the same month. If you're on March 31, it jumps to March 3. (Users would probably expect that if they're on the last day of the month, they jump to the last day of the next/prev month.)

Here are GIFs of the arrow keys in Chrome/macOS. I'm pressing down twice, then up twice.

CleanShot 2021-07-09 at 08 11 12

Now I'm pressing left twice (skips a day), right twice (doesn't move).

CleanShot 2021-07-09 at 08 12 46

UPDATE: the same behavior occurs on Safari and Firefox on macOS.

@claviska
Copy link
Member

claviska commented Jul 9, 2021

so maybe it would be an idea to start "fully RTL support" with the date picker to gain some experiences

RTL will come after the theme updates. I've seen helper functions that make it a lot easier to do, and those will be much easier to implement in CSS-in-JS (i.e. Lit's template literal syntax) than in Sass.

@hanc2006
Copy link
Author

hanc2006 commented Jul 9, 2021

@claviska
about months and years navigation. I thought something like this:

For keyboard accessibility? What shortcut could I use??

@claviska
Copy link
Member

claviska commented Jul 9, 2021

I would look at Duet: https://duetds.github.io/date-picker/

In fact, my original plan was to fork this one for Shoelace. They've ran it through an accessibility audit so it's pretty bulletproof from an a11y standpoint.

They don't support date range, but they do support disabling individual days.

@hanc2006
Copy link
Author

hanc2006 commented Jul 9, 2021

The last commit

They don't support date range, but they do support disabling individual days.

The ability to disable days is in work progress, but this will only be possible when range mode is disabled.

I would look at Duet: https://duetds.github.io/date-picker/

I will work on this version also, by now I finished the first above.

…ated calendar facade, improved component style
@hanc2006
Copy link
Author

hanc2006 commented Jul 10, 2021

This commit d7910e8 adds some new features:

  • Now you can browse months and years much easier using the new calendar views
  • Navigation between months and years respects the min and max date restrictions
  • The default calendar sizes has been resized (more compact)
  • Consolidate the calendar base class for dates manipulation
simplescreenrecorder-2021-07-10_20.38.15.mp4

@hanc2006
Copy link
Author

Disabled dates feature is now available 👍

image

@hanc2006
Copy link
Author

Limit dates selection feature is now available +1

image

@hanc2006
Copy link
Author

Closed in favor of this new PR #486.

@hanc2006 hanc2006 closed this Jul 23, 2021
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