feat: add date metadata provider to date-picker#12041
Conversation
web-padawan
left a comment
web-padawan
left a comment
There was a problem hiding this comment.
Let's discuss this with the team if we want to deprecate isDateDisabled API and remove in V26.
We should not have 2 different APIs for the same feature if one of them doesn't work with Flow.
Regarding the PR itself: it adds multiple things like a new loader part in the overlay content.
Also it adds features to both date-picker and date-time-picker (including isDateDisabled).
I would highly recommend to consider splitting it further once we agree about the API shape.
Sure, but Flow is not really an issue here. For Flow, there is three variants that you can use right now, see vaadin/flow-components#9691 (comment) |
Sure, that's easy once it's clear what to split out. I can restrict this to adding to only date picker first if that makes things easier. |
To reduce the changes in #12041
Beyond the existing per-date isDateDisabled predicate, add a disabledDatesProvider callback that is consulted for a whole range of dates at once. This lets disabled dates be resolved lazily, for example from a backend, without one round-trip per date: a controller caches resolved months and prefetches around the visible range, and a spinner is shown while a range loads. isDateDisabled and disabledDatesProvider are also forwarded to the date-picker inside date-time-picker, where they previously had no effect. Fixes #1820 Part of #2867
The while loops mutated the loop variable via date.setDate(), which Sonar (rule S2189) does not recognise as modifying the condition and reports as a potential infinite loop. Use a counted for loop instead.
The async provider tests asserted after a fixed two renders, but the resolve -> version bump -> calendar re-render chain can take more renders on Firefox. Wait until the expected state is reached instead.
a7f98cf to
7aafe21
Compare
The loader part and loading attribute indicate that data is being loaded; the disabled dates provider is currently the only trigger, so describe it as an example rather than the definition.
|
Some review findings from Claude based on the current branch shape:
|
Should there be a timeout? Should it allow selection of all dates if the server is slow? |
Why is this a problem for "future Flow"? It should never change the provider as far as I know |
Artur-
changed the title
Move the disabled dates controller from the overlay content to the date-picker so validation consults the provider even when the overlay is never opened. A value typed with auto-open disabled or set programmatically is now rejected once the provider answers.
When the overlay opens without a selected value, move the auto-picked initial focus to the closest selectable date once the provider answers for that month. Only the auto-picked date is adjusted, and only while the user has not navigated away, so disabled dates stay keyboard-focusable.
Keep overlayContentStyles as a single template and add the shared loaderStyles via the component's styles array, instead of importing it into the base styles file. Shrinks the base styles diff to the two rules the loader actually needs.
|
Fixed 1 and 2 |
Remove empty hostConnected, use Array.at(-1), and prefer optional chaining. No behavior change.
Month calendar now reads disabled/loading state directly from the shared controller instead of mirroring it into local __disabledDatesSet and __loadingDisabledDates fields. Fold the repeated 'disabled or still loading' check into controller.isDateBlocked().
Publish per-commit dev versions (<base>-dev.<hash>) of all packages and move a per-feature dist-tag to the newest build, giving Maven -SNAPSHOT style 'always latest' installs via npm i @vaadin/<pkg>@Dev-<feature>.
lerna publish from-package refuses to run on a dirty tree, and the version bump in steps 2-3 leaves package.json/lerna.json/define.js modified. Add a throwaway local commit (never pushed) before publishing.
A missing or invalid token makes npm publish return a misleading E404. Check npm whoami up front (real npmjs publish only) so the run stops with a clear message before the version bump and build, not after.
The publish was hitting E404 because auth was wired to NODE_AUTH_TOKEN while the environment provides the token as NPM_TOKEN. Read NPM_TOKEN (the standard npm CI variable) in the script, write an .npmrc that references it, and fall back to an existing npm login. The TeamCity conf no longer sets up .npmrc.
|
Do we actually need disabled dates provider API in the web component? It adds a lot of complexity and proposed |
|
Of course we could implement something else but why? There are clear use cases for disabling arbitrary dates |
|
Wouldn't The whole controller thing (month cache & invalidation, debouncing etc) looks quite convoluted and fragile, and is a potential source of bugs. I would prefer to start with a minimal implementation unless it's absolutely needed. A simpler prototype branch: https://github.com/vaadin/web-components/tree/proto/disabled-dates |
|
For which range would you fetch disabled dates if there is only a push mechanism and no pull? Especially if the operation happens to be costly? If you fetch for only the current month, what do you do with the rest of the dates? |
|
The current solution feels like it's forcing functionality onto an architecture that wasn't built for it: DatePicker was never designed to load and render dates lazily. Disabled dates is just one case where this need arises, and I can imagine more potential features that would need the same, e.g. adding custom part names to dates to style them differently. This makes me believe that the entire approach to how DatePicker gets dates needs a rethink. Perhaps this calls for some sort of data provider concept that would let DatePicker fetch dates or months from a backend asynchronously. The backend could then return additional metadata for each date, such as its disabled state or part names, and the existing synchronous generators could then use that metadata as extra context, with no need for new range-handling generators. After all, that's our standard pattern for lazy collections: both ComboBox and Grid work this way. |
|
Yes but it is not loading dates lazily - there is no custom data to render involved. The generalization is more that it is loading metadata for dates on demand - the logical next thing to want would indeed be classname for specific dates to show e.g. if a date is fully available, almost fully booked or not available at all (disabled and classname). The design could be adapted to that and made extensible in the future by renaming the method and returning an object instead of a single boolean |
Rename disabledDatesProvider to dateMetadataProvider and return per-date metadata objects instead of a plain disabled-dates list. Each entry is a DatePickerDate extended with metadata (disabled, part), so the same lazy fetch-per-range mechanism can drive more than just disabled state. Add custom part names per date: metadata part tokens are added to the date cell's part attribute, so a theme can style specific dates via ::part(). The controller now caches a metadata map (keyed by date) rather than a set of disabled keys; isDateDisabled derives from metadata.disabled. Rename the controller to DateMetadataController and the tests accordingly.
Note that part names returned by dateMetadataProvider are added to date cells and can be styled via vaadin-month-calendar::part(<name>). Also add the pending part to the .d.ts table and align the pending description with the renamed provider.
Artur-
changed the title
Match the clearCache method that grid and combo-box expose on their data providers. Clears the cached date metadata and reloads it for the visible range and the selected value, so applications can refresh after the data behind dateMetadataProvider changes without replacing the provider.
Note that dateMetadataProvider returns both disabled and part together, rather than via separate generators, so a single backend query (Flow) can answer both. Also point to clearCache for reloading.
|
|
Updated to a metadata provider and to support assigning part names to individual dates. Also added a similar Do you think this design makes sense? |
3 participants
Applications need to disable arbitrary dates in the date-picker — weekends, public holidays, or dates that are already booked — and increasingly to style specific dates (for example to mark a day as busy or almost fully booked). Listing every such date up front does not scale: the set can be large, unbounded into the future, or only known to a server. This adds a way to load that information for the dates the calendar is about to show, on demand, the same way
<vaadin-grid>and<vaadin-combo-box>load their items.A new
dateMetadataProviderfunction is called with the range of dates the calendar is about to render and returns, or resolves with, metadata for the dates in that range. Each entry is aDatePickerDateextended with metadata —disabledand custompartnames, e.g.{ year, month, day, disabled: true, part: 'busy' }. This keeps the synchronousisDateDisabledpredicate and themin/maxconstraints; a date is disabled if it is out of range, orisDateDisabledreturnstrue, or its metadata marks it disabled.Changes
dateMetadataProviderproperty on<vaadin-date-picker>, plus the publicDatePickerDateRangeandDatePickerDateMetadatatypes. The provider may return an array synchronously or aPromise, so results can come from a server or a remote service.DateMetadataController) owns the metadata. It calls the provider once per range of months (never one date at a time), caches resolved months so scrolling back and forth does not re-fetch, and prefetches a buffer of months around the visible range. Loads triggered by scrolling are debounced.loadingattribute, reusing the shared loader styles) and marks itself busy for assistive technology. Dates in a month that is still loading render in a non-selectablependingstate, so they are never shown as selectable and then flipped to disabled.partmetadata is added to the matching date cells, so a theme can style specific dates withvaadin-month-calendar::part(<name>).Only
<vaadin-date-picker>is covered here;<vaadin-date-time-picker>forwarding and the Flow API are handled separately so this change stays reviewable on its own.Fixes #1820, Part of #2867