feat: add DatePickerEx component #96
Conversation
WalkthroughThis update introduces new date picker functionality with enhanced styling and increment/decrement controls. A new LitElement-based web component for date picker buttons is added, and a custom date picker JavaScript component ( Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
javier-godoy
force-pushed
the
DatePickerEx
branch
from
f86a297 to
9d0f1ff
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js (2)
22-27: Cursor feedback is missingConsider adding
cursor: pointer;to thebuttonrule so users get visual feedback that the arrows are clickable.
41-46: Custom events are not bubbling or crossing the shadow DOM
new CustomEvent('click-up')does not bubble or compose by default.
Even though the parent element attaches the listener directly onbuttons, enablingbubbles: true, composed: trueprovides future‑proofing if the listener ever moves to an ancestor.-this.dispatchEvent(new CustomEvent("click-up")); +this.dispatchEvent(new CustomEvent("click-up", {bubbles: true, composed: true}));Apply the same to
"click-down".src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java (1)
76-86: Potentially large client round‑trip per focus
fetchStylesiterates month‑by‑month betweenminandmax, which can be up to 50 months in the current caller logic.
If the generator is expensive (e.g. hits a DB/REST service) this can degrade performance.Consider:
- Caching the resulting JSON in the server component (e.g.
ConcurrentHashMap<YearMonth, JsonObject>).- Allowing the client to request only the month currently in view instead of a window.
src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java (1)
47-55: Minor optimisation for the class name generator
date.getDayOfWeek()is calculated twice. Store it once for readability:- if (date.getDayOfWeek() == DayOfWeek.SATURDAY || date.getDayOfWeek() == DayOfWeek.SUNDAY) { + DayOfWeek dow = date.getDayOfWeek(); + if (dow == DayOfWeek.SATURDAY || dow == DayOfWeek.SUNDAY) {src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
11-18: Staticsuper.templateaccess flagged by linterBiome warns that
superin a static context can be confusing (line 13).
Using the explicit parent class (DatePicker.template) avoids the warning and
makes the inheritance clearer:- ${super.template} + ${DatePicker.template}🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🛑 Comments failed to post (4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js (1)
10-12: 🛠️ Refactor suggestion
Add accessibility & form‑safety attributes to the buttons
Both buttons lack an explicit
type(defaulting to submit inside a form) and have no accessible label. This can cause:
- Unexpected form submission when the picker lives inside a
<form>.- Screen‑reader users receiving unnamed/undiscoverable controls.
A minimal fix:
- <button id="up" @click=${this._clickUp}></button> - <button id="down" @click=${this._clickDown}></button> + <button id="up" + type="button" + aria-label="Increment" + @click=${this._clickUp}></button> + <button id="down" + type="button" + aria-label="Decrement" + @click=${this._clickDown}></button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<button id="up" type="button" aria-label="Increment" @click=${this._clickUp}></button> <button id="down" type="button" aria-label="Decrement" @click=${this._clickDown}></button> `;src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java (1)
71-74: 💡 Verification agent
🧩 Analysis chain
refreshAll()is a no‑op
refreshAll()is invoked after setting a newclassNameGenerator, but it currently comments out the only statement. That leaves existing overlays styled with stale data.Either implement the intended element JS call or remove the method to avoid giving a false sense of refresh behaviour.
🏁 Script executed:
#!/bin/bash # Search for usages of refreshAll across the repo rg "refreshAll" -n . # Search for classNameGenerator in DatePickerEx.java rg "classNameGenerator" -n src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java # Show context around setClassNameGenerator override in DatePickerEx.java rg -n "setClassNameGenerator" -C3 src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.javaLength of output: 2160
Implement or remove the no‑op refreshAll() in DatePickerEx
The override in src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java (lines 71–74) is effectively empty—the only statement (
getElement().executeJs("…")) is commented out. BecausesetClassNameGenerator(...)callsrefreshAll(), new generators won’t be applied to existing date cells.• File to update:
- src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
Suggested fixes:
- Re‑enable or replace the JavaScript call inside
refreshAll()to clear and re‑apply CSS classes, for example:public void refreshAll() { getElement().executeJs("setTimeout(()=>this._clearEmptyDaysStyle())"); }- Or remove this override entirely if the base class’s implementation already covers your use case—and adjust/remove any calls to
refreshAll()if unnecessary.src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (2)
55-66:
⚠️ Potential issueIncorrect key used when probing the style‑cache – breaks pre‑fetch logic
this._stylesis keyed with the string"YYYY-MM", but the probe inside the search window uses the Date object itself:if(this._styles[min]) // and maxBecause
min/maxareDateinstances, the look‑ups always fail, causing
unnecessary server calls and preventing the early‑exit optimisation.-if(this._styles[min]) {min=add(min,+1); break;} +if(this._styles[tostr(min)]) {min=add(min,+1); break;} ... -if(this._styles[max]) {max=add(max,-1); break;} +if(this._styles[tostr(max)]) {max=add(max,-1); break;}
106-165: 🛠️ Refactor suggestion
Date component increment can yield invalid ISO dates
incrementComponentrewrites the ISO string by simple arithmetic, so
2025-01-31→ increment month →2025-02-31(invalid)2024-02-29→ increment year →2025-02-29(invalid on non‑leap year)Setting
this.valueto an invalid string leaves the field blank and may emit an error event.A safer approach is to operate on a
Dateobject and normalise via
setFullYear,setMonth,setDate, letting the browser handle overflow:- this.value = incrementComponent(this._formatISO(date), part, delta); + const d = new Date(date); + if (part === 'Y') d.setFullYear(d.getFullYear() + delta); + if (part === 'M') d.setMonth(d.getMonth() + delta); + if (part === 'D') d.setDate(d.getDate() + delta); + this.value = this._formatISO(d);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const incrementComponent = (value, part, delta) => { // Increments the numeric component (year, month, or day) of a date string based on the current component index. switch (part) { case 'Y': return incrementSubstring(value, 0, 4, delta); case 'M': return incrementSubstring(value, 5, 7, delta); case 'D': return incrementSubstring(value, 8, 10, delta); } }; - this.value = incrementComponent(this._formatISO(date), part, delta); + const d = new Date(date); + if (part === 'Y') d.setFullYear(d.getFullYear() + delta); + if (part === 'M') d.setMonth(d.getMonth() + delta); + if (part === 'D') d.setDate(d.getDate() + delta); + this.value = this._formatISO(d); // Find the start of the index-th component let start = 0; for (let i = 0; i < index; /*increment in loop*/) { if (!this.inputElement.value[start].match(/[0-9]/)) i++; start++; }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
91-103: Overlay renderer monkey‑patching could break after Vaadin upgradesOverriding the private
_createElementon the internal month scroller tightly couples the component to Vaadin internals and risks breakage on library updates.Consider using the public overlay renderer callback (
overlayClass.itemsRendered) or, if unavailable, file a feature request upstream to expose a hook for day cell post‑processing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xml(1 hunks)src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pom.xml
- src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
- src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java
🧰 Additional context used
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (2)
40-47: Potential NPE when day cell is not present
_setStyleForDayassumes thatquerySelectorAll("[part~='date']:not(:empty)")[i‑1]always returns an element, but Vaadin omits cells for overflow days in the first/last week.
Accessinge.classNamewheneisundefinedwill throw.- let e = this.shadowRoot.querySelectorAll("[part~='date']:not(:empty)")[i-1]; - if (className) { - e.className = className; - } else { - e.removeAttribute('class'); - } + const dates = this.shadowRoot.querySelectorAll("[part~='date']:not(:empty)"); + const e = dates[i - 1]; + if (!e) return; // day outside current month + if (className) { + e.className = className; + } else { + e.removeAttribute('class'); + }Prevents a run‑time error when generators set styles for “missing” days (e.g. 31st in a 30‑day month).
11-18: Static template usessuper.template; Biome warns but usage is validBiome flags “Using
superin a static context can be confusing”, yet here it is the canonical pattern recommended by Vaadin to extend templates. No action required.🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
Outdated
Show resolved
Hide resolved
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
DatePickerEx
branch
from
9d0f1ff to
d54fc68
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (4)
11-22: Consider eliminating thesuper.templatereference inside a static getterStatic analysis (Biome) warns that using
superin a static context is potentially confusing.
While Polymer still resolves this correctly, an explicit reference to the parent class can increase clarity and avoid future framework surprises.- ${super.template} + ${DatePicker.template}🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
40-47: Defensive check for out-of-range day indices
_setStyleForDayassumesiis 1-based and that the queried element exists.
When the server delivers an unexpected key (e.g. day ‘31’ on a 30-day month)ebecomesundefined, raising a runtime error.- let e = this.shadowRoot.querySelectorAll("[part~='date']:not(:empty)")[i-1]; - if (className) { - e.className=className; - } else { - e.removeAttribute('class'); - } + const dayEls = this.shadowRoot.querySelectorAll("[part~='date']:not(:empty)"); + const e = dayEls[i - 1]; + if (!e) return; // guard against out-of-range indices + if (className) { + e.className = className; + } else { + e.removeAttribute('class'); + }
91-103: Overriding_createElementrepeatedly can leak memory
this._overlayContent._monthScroller._createElementis reassigned every time the overlay renders.
On reopenings of the picker the wrapper wraps an already-wrapped function, growing the call stack and preventing GC of old closures.Add an idempotent guard:
if (!this._overlayContent._monthScroller.__fcWrapped) { const createElement = this._overlayContent._monthScroller._createElement; this._overlayContent._monthScroller._createElement = () => { /* … */ }; this._overlayContent._monthScroller.__fcWrapped = true; }
106-157: Verify caret-preservation logic after value update
this.value = …triggers Vaadin’s internal re-render.
The new text might not be present ininputElement.valueimmediately, so the subsequentselectionStart/Endcalculations can be off by one frame, causing the caret to jump.Please test in slow networks/locales with long formats (e.g. “31.12.2024”).
If issues arise, wrap the caret restoration inrequestAnimationFrame:this.value = incrementComponent(date, part, delta); requestAnimationFrame(() => { /* recompute start/end & set selection */ });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
- src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java
🧰 Additional context used
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (1)
src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java (1)
42-42: New demo registration looks correct
DatePickerDemois now part of the tab set in a logical position between Month- and InlineDatePicker demos.
No further changes required.
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
Outdated
Show resolved
Hide resolved
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
Outdated
Show resolved
Hide resolved
javier-godoy
force-pushed
the
DatePickerEx
branch
from
d54fc68 to
9cc4d1e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (5)
11-23: Template override looks good but there's an analyzer warning.The static analysis tool flagged
${super.template}at line 13 with "Using super in a static context can be confusing." While this is a common pattern in Polymer components and works correctly here, you might consider using the explicit parent class name to silence the warning.static get template() { return html` - ${super.template} + ${DatePicker.template} <style> vaadin-input-container { padding-left: 0; } ::slotted(input) { padding-left: calc(0.575em + var(--lumo-border-radius-m) / 4 - 1px); } </style> `; }🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
25-112: Consider moving helper functions to the class prototype.The helper functions
_clearStyles,_setStyleForDay,_getStyle, and_updateMonthStylesare defined inside theready()method, which means they're recreated for each instance. Moving them to the class prototype would be more efficient, especially for applications with multiple instances.export class FcDatePicker extends DatePicker { // ...existing code... + _clearStyles() { + let e = this.shadowRoot.querySelectorAll("[part~='date']"); + e.forEach(item => item.removeAttribute('class')); + } + + _setStyleForDay(i, className) { + let e = this.shadowRoot.querySelectorAll("[part~='date']:not(:empty)")[i-1]; + if (className) { + e.className = className; + } else { + e.removeAttribute('class'); + } + } + + _getStyle(month) { + const tostr = date => date.toISOString().substr(0,7); + const add = (date, delta) => new Date(date.getFullYear(), date.getMonth()+delta, 1); + + const key = tostr(month); + + if (!this._styles[key]) { + // ...existing implementation... + } + return this._styles[key]; + } + + _updateMonthStyles(element) { + const month = element.month; + this._clearStyles.call(element); + this._getStyle(month).then(styles => setTimeout(() => { + if (element.month === month) { + for (let i in styles) { + this._setStyleForDay.call(element, i, styles[i]); + } + } + })); + } ready() { super.ready(); // ...existing button setup code... this._styles = {}; - const _clearStyles = function() { - // ...existing implementation... - }; - - const _setStyleForDay = function(i, className) { - // ...existing implementation... - }; - - const _getStyle = month => { - // ...existing implementation... - } - - const _updateMonthStyles = function(element) { - // ...existing implementation... - } this._overlayElement.renderer = e => { this._boundOverlayRenderer.call(this, e); if (!this._overlayContent._monthScroller.__fcWrapped) { const createElement = this._overlayContent._monthScroller._createElement; this._overlayContent._monthScroller.__fcWrapped = true; this._overlayContent._monthScroller._createElement = () => { var calendar = createElement(); calendar.addEventListener('dom-change', ev => { if (ev.composedPath()[0].as == 'week') { - setTimeout(() => _updateMonthStyles(calendar)); + setTimeout(() => this._updateMonthStyles(calendar)); } }); return calendar; } } }; } }
98-110: Consider using a WeakMap for tracking wrapped elements.The current implementation adds a non-standard property
__fcWrappeddirectly to the scroller object. Using a WeakMap would be a cleaner approach that avoids potentially conflicting with future Vaadin implementations.export class FcDatePicker extends DatePicker { + static _wrappedScrollers = new WeakMap(); // ...existing code... ready() { // ...existing code... this._overlayElement.renderer = e => { this._boundOverlayRenderer.call(this, e); - if (!this._overlayContent._monthScroller.__fcWrapped) { + if (!FcDatePicker._wrappedScrollers.has(this._overlayContent._monthScroller)) { const createElement = this._overlayContent._monthScroller._createElement; - this._overlayContent._monthScroller.__fcWrapped = true; + FcDatePicker._wrappedScrollers.set(this._overlayContent._monthScroller, true); this._overlayContent._monthScroller._createElement = () => { // ...existing code... } } }; } }
120-132: Improve readability with named constants for the format detection.The date format detection logic is quite complex. Consider adding more comments or extracting the complex regex into a named constant for better maintainability.
// Count how many non-digit characters appear before the current cursor position let index = 0; for (let i = 0; i < this.inputElement.selectionStart; i++) { if (!this.inputElement.value[i].match(/[0-9]/)) index++; } // Map the component index to its corresponding date part character + // First create a template with Y, M, D characters in place of digits const format = this.__formatDate(new Date(3333, 10, 22)) .replaceAll('1', 'M') .replaceAll('2', 'D') .replaceAll('3', 'Y'); + // Regular expression to find which component (Y, M, or D) the cursor is in + const componentRegex = new RegExp(`^([DMY]+[^DMY]){${index}}`); - const part = format.replace(new RegExp(`^([DMY]+[^DMY]){${index}}`),'')[0]; + const part = format.replace(componentRegex, '')[0];
156-158: Consider refactoring to use regex for finding component boundaries.The code to find the end of the index-th component could be simplified using a regex.
// Find the end of the index-th component let end = start; - while (end < this.inputElement.value.length && this.inputElement.value[end].match(/[0-9]/)) { - end++; - } + const digitMatch = this.inputElement.value.substring(start).match(/^\d+/); + end = digitMatch ? start + digitMatch[0].length : start;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java
- src/test/java/com/flowingcode/addons/ycalendar/DatePickerDemo.java
- src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
- src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[error] 13-13: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (2)
69-72: Good error handling for promise rejection.You've properly addressed the previous review comment by adding error handling to the
fetchStylespromise with a.catch()block. This prevents unhandled promise rejections from causing issues and provides useful error logging.
134-144: Good implementation of date component increment.You've properly addressed the previous review comment by using Date API methods to handle date increments, which correctly handles edge cases like month rollovers and invalid dates.
src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
Outdated
Show resolved
Hide resolved
src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
Outdated
Show resolved
Hide resolved
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
Show resolved
Hide resolved
src/main/java/com/flowingcode/addons/ycalendar/DatePickerEx.java
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from To Do
to In Progress
in Flowing Code Addons
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
155-159: Consider a more reliable approach for determining date parts.The current regex-based approach to identify date parts might not work correctly with all locales or formatting patterns. As noted in previous reviews and learning, this add-on only supports Gregorian calendars, but the approach could be improved for better reliability.
- const format = this.__formatDate(new Date(3333,10,22)) - .replaceAll('1','M') - .replaceAll('2','D') - .replaceAll('3','Y'); - const part = format.replace(new RegExp(`^([DMY]+[^DMY]){${index}}`),'')[0]; + // Format dates that differ in only one component + const baseDate = new Date(2020, 1, 2); + const yearDate = new Date(2021, 1, 2); // Only year changed + const monthDate = new Date(2020, 2, 2); // Only month changed + const dayDate = new Date(2020, 1, 3); // Only day changed + + const baseFormat = this.__formatDate(baseDate); + const yearFormat = this.__formatDate(yearDate); + const monthFormat = this.__formatDate(monthDate); + const dayFormat = this.__formatDate(dayDate); + + // Identify which positions change for each component + let parts = []; + for (let i = 0; i < baseFormat.length; i++) { + if (yearFormat[i] !== baseFormat[i]) { + parts.push({pos: i, type: 'Y'}); + } else if (monthFormat[i] !== baseFormat[i]) { + parts.push({pos: i, type: 'M'}); + } else if (dayFormat[i] !== baseFormat[i]) { + parts.push({pos: i, type: 'D'}); + } + } + + // Group consecutive positions of the same type + let groups = []; + parts.sort((a, b) => a.pos - b.pos).forEach(part => { + if (!groups.length || groups[groups.length-1][0].type !== part.type) { + groups.push([part]); + } else { + groups[groups.length-1].push(part); + } + }); + + // Find which part contains our cursor + const cursorPos = this.inputElement.selectionStart; + const part = groups.find(g => + cursorPos >= g[0].pos && cursorPos <= g[g.length-1].pos + 1 + )?.[0]?.type || groups[0][0].type;
🧹 Nitpick comments (3)
src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (1)
43-45: Consider adding more descriptive notification content.The value change listener only shows the raw date value. Consider adding a more descriptive message to make the notification more user-friendly.
field.addValueChangeListener(ev->{ - Notification.show(Objects.toString(ev.getValue())); + Notification.show("Date changed to: " + Objects.toString(ev.getValue())); });src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (1)
44-44: Consider adding @nullable annotation for the class name generator.Since the class name generator can be null, consider adding the @nullable annotation for better IDE support and code clarity.
-private ValueProvider<LocalDate, String> classNameGenerator; +private @Nullable ValueProvider<LocalDate, String> classNameGenerator;Don't forget to add the import:
import javax.annotation.Nullable;src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
30-42: Be explicit about parent template in static context.Using
super.templatewithin a static method can be confusing. It would be better to explicitly reference the parent class.static get template() { return html` - ${super.template} + ${DatePicker.template} <style> vaadin-input-container { padding-left: 0; } ::slotted(input) { padding-left: calc(0.575em + var(--lumo-border-radius-m) / 4 - 1px); } </style> `; }🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java
- src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
🧰 Additional context used
🧠 Learnings (1)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
Learnt from: javier-godoy
PR: FlowingCode/YearMonthCalendarAddon#96
File: src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js:128-132
Timestamp: 2025-04-24T18:03:00.778Z
Learning: The YearMonthCalendarAddon only supports Gregorian calendars and does not support non-Gregorian calendars such as Hijrah, as documented in issue #20.
🧬 Code Graph Analysis (2)
src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (1)
src/test/java/com/flowingcode/addons/ycalendar/TestUtils.java (1)
TestUtils(30-120)
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (1)
src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java (1)
SuppressWarnings(30-48)
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[error] 32-32: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (12)
src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (3)
32-34: Good route path and title convention.The demo is properly configured with
@PageTitle("DatePicker")and organized under the "year-month-calendar/date-picker" route path with the correct parent layout.
38-41: Conditional locale selector inclusion is well-implemented.The preprocessor directive will only include the LocaleSelector component when appropriate for the Vaadin version, which is a good approach for version compatibility.
47-55: Well-implemented class name generator for date styling.The class name generator efficiently handles weekend and holiday styling. The code correctly identifies weekends based on DayOfWeek and applies the "weekend" class, while using TestUtils.isPublicHoliday to identify holidays for the "holiday" class.
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (4)
33-42: Excellent class documentation and proper component registration.The class is well-documented with clear purpose description and properly registered as a custom element with the correct tag name and JavaScript module.
64-75: Well-documented public API with clear behavior description.The setClassNameGenerator method is well-documented, clearly explaining the behavior when null is returned from the generator and how multiple class names can be provided. The refreshAll() call ensures that UI is updated immediately.
82-92: Good implementation of client-callable method with date range processing.The fetchStyles method efficiently processes a range of months, avoiding unnecessary operations by only adding month styles when present.
94-103: Efficient style generation with proper Optional handling.The getStyles method makes good use of Java 8's Optional and stream features to cleanly generate styles for each day of the month, returning an empty Optional when no styles are needed.
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (5)
44-52: Good lifecycle practice with proper event listeners.Calling
super.ready()first is the correct approach. The event listeners for the buttons are properly configured.
55-71: Clever approach to extending the calendar with style support.The overlay renderer override wraps the month scroller's createElement method to add style support without duplicating the entire renderer logic. The use of timeout in the dom-change event handler is appropriate to ensure the DOM is fully updated.
97-128: Well-implemented style caching with range fetching.The
_getStylefunction efficiently caches styles and fetches ranges of months to minimize server requests. The error handling for the server call is properly implemented.
161-171: Good implementation of date component increment.The incrementComponent function correctly handles field rollover and clamping by using the native Date API, which ensures valid dates are always generated.
177-185: Effective cursor positioning after date change.The code correctly identifies the boundaries of the numeric components in the formatted date string, allowing the cursor to be placed back in the same component after incrementing, which provides an excellent user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (2)
44-44: Consider making the field final.Since this field is only set in the
setClassNameGeneratormethod and doesn't need to be reassigned elsewhere, marking it asfinalcould help with code clarity.- private ValueProvider<LocalDate, String> classNameGenerator; + private final AtomicReference<ValueProvider<LocalDate, String>> classNameGenerator = new AtomicReference<>();Then update the setter method to use
classNameGenerator.set(generator)and getter references toclassNameGenerator.get().
72-75: Consider adding a getter for the classNameGenerator.For consistency with JavaBeans patterns and to allow retrieving the current generator, consider adding a getter method.
+ /** + * Gets the current class name generator for this calendar. + * + * @return the current {@code ValueProvider} used for generating class names, or null if none is set + */ + public ValueProvider<LocalDate, String> getClassNameGenerator() { + return this.classNameGenerator; + }src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (1)
36-41: Consider extracting conditional code to a separate method.The conditional inclusion of the
LocaleSelectorwith preprocessor directives makes the code harder to read. Consider extracting this pattern to a utility method if it's common across your demo classes.src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (1)
155-159: Potential fragility in date part detection.The approach to determine which date part the cursor is on relies on specific digit positions in the formatted date. While this works for Gregorian calendars and common locales, it might not be robust across all formatting patterns.
Consider a more reliable approach that analyzes formatting differences between dates that vary only in one component.
// Format dates that differ in only one component const baseDate = new Date(2020, 1, 2); const yearDate = new Date(2021, 1, 2); // Year changed const monthDate = new Date(2020, 2, 2); // Month changed const dayDate = new Date(2020, 1, 3); // Day changed // Compare formatted strings to find changing positions const baseStr = this.__formatDate(baseDate); const yearStr = this.__formatDate(yearDate); const monthStr = this.__formatDate(monthDate); const dayStr = this.__formatDate(dayDate); // Identify changed character positions for each component const parts = {}; for (let i = 0; i < baseStr.length; i++) { if (yearStr[i] !== baseStr[i]) parts[i] = 'Y'; else if (monthStr[i] !== baseStr[i]) parts[i] = 'M'; else if (dayStr[i] !== baseStr[i]) parts[i] = 'D'; } // Find component at cursor position const cursorPos = this.inputElement.selectionStart; const part = Object.entries(parts).reduce((result, [pos, type]) => (cursorPos >= Number(pos) && cursorPos <= Number(pos) + 1) ? type : result, 'D');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
pom.xml(2 hunks)src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js(1 hunks)src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java(1 hunks)src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- pom.xml
- src/test/java/com/flowingcode/addons/ycalendar/YearMonthCalendarDemoView.java
- src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker-buttons.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (1)
src/test/java/com/flowingcode/addons/ycalendar/TestUtils.java (1)
TestUtils(30-120)
🪛 Biome (1.9.4)
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js
[error] 32-32: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (13)
src/main/java/com/flowingcode/addons/ycalendar/ExtendedDatePicker.java (4)
33-38: Well-structured component with clear documentation.The class extends DatePicker with two key features - UI controls for date part adjustment and dynamic CSS styling based on date values. The Javadoc clearly explains this functionality.
39-41: Appropriate component setup with necessary annotations.The annotations correctly define a custom tag name that matches the JavaScript implementation and properly reference the JS module that implements the client-side behavior.
82-92: Good date range handling in fetchStyles method.The client-callable method properly parses the date range, iterates through each month, and collects styles into a JSON response. This provides an efficient way for the client to fetch styling data for multiple months in a single request.
94-103: Efficient style collection implementation.The
getStylesmethod efficiently builds a JSON object containing day-specific styles and only returns a populated result when at least one day has a style. The use ofOptionalfor handling potential empty results is a good practice.src/test/java/com/flowingcode/addons/ycalendar/ExtendedDatePickerDemo.java (3)
31-34: Appropriate demo configuration with routing metadata.The class is properly annotated with necessary Vaadin routing information and integrated into the demo application structure.
43-45: Good demonstration of value change event handling.The demo properly showcases how to handle value changes by adding a listener that displays a notification with the selected date.
47-55: Excellent demonstration of the classNameGenerator functionality.The implementation shows a practical use case by styling weekends and holidays differently. This clearly demonstrates the main feature of the ExtendedDatePicker component.
src/main/resources/META-INF/frontend/fc-date-picker/fc-date-picker.js (6)
25-29: Component properly extends Vaadin DatePicker.The component is correctly set up with a custom element name that matches the Java @tag annotation.
30-42: Caution: Using super in static context.Using
superin a static method can be confusing sincesupertypically refers to an instance relationship. However, this pattern is commonly used in Polymer components so it's acceptable in this context.🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Using super in a static context can be confusing.
super refers to a parent class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
44-72: Well-structured component initialization.The ready method properly initializes the component by:
- Calling super.ready() first (which is important for lifecycle ordering)
- Adding and configuring the custom buttons
- Setting up the styles cache
- Customizing the overlay renderer
This ensures all functionality is properly initialized.
97-128: Robust style caching with error handling.The _getStyle method efficiently:
- Implements a caching strategy to minimize server requests
- Handles boundary cases by looking for existing styles in adjacent months
- Properly handles promise rejections to prevent unhandled promise errors
- Uses a shared fetch request for multiple months
This implementation balances performance with reliability.
161-171: Good implementation of date component adjustment.The incrementComponent function correctly uses the Date API to handle field rollover and clamping, ensuring that invalid dates are never created (e.g., when incrementing month from 12 to 13 or day from 31 to 32).
173-191: Well-implemented cursor positioning after date change.The code carefully preserves the cursor position to keep it within the same date component after the value is updated, providing a seamless user experience when incrementing/decrementing date parts.
|
@javier-godoy I tested the component and it works really great. It's a really good feature. I do have a comment regarding the look of the arrows, for me they look too dark and I'm not a fan of them having a border. I believe that they can be styled but what if we can have a better default? I did some browser testing and I think something like this looks better and more "Vaadin like" :
These are the styles I tried: |
|
github-project-automation
bot
moved this from In Progress
to Pending release
in Flowing Code Addons
Summary by CodeRabbit
New Features
Documentation
Chores