Add overflow button when component is too narrow #14
Conversation
WalkthroughVersion bumped to 1.2.0-SNAPSHOT. DayOfWeekSelector refactored into a Vaadin web component with programmatic DayOfWeekButton management, client-callable toggles, new APIs (overflow icon, I18N, constructors), and a LitElement frontend that handles responsive overflow. CSS and demo/test styles/layout updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (1)
207-212: Toggle has-label only when label is non-emptyCurrent logic sets the attribute even for empty strings. Use a trimmed check.
- if (_changedProperties.has('label')) { - this.toggleAttribute('has-label', this.label != null); - } + if (_changedProperties.has('label')) { + const has = typeof this.label === 'string' && this.label.trim().length > 0; + this.toggleAttribute('has-label', has); + }src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (1)
75-76: ClientCallable visibility
@ClientCallable private void toggleState()is callable, but usingprivatehere is unconventional and may hinder future refactors. Consider making itpublicfor clarity and consistency with Vaadin examples.src/test/resources/META-INF/frontend/styles/shared-styles.css (3)
21-22: Remove empty html ruleThis rule is a no-op and adds noise.
-html { -}
24-26: Scope the demo gap rule to the component elementTo avoid accidentally applying gap to non-layout elements that may also use the "demo" class, target the tag explicitly. This also clarifies the intent that the rule is for VerticalLayout instances.
-.demo { +vaadin-vertical-layout.demo { gap: var(--lumo-space-m); }
28-30: Use Lumo design token for font weightAligns with theming and makes the style consistent with Lumo’s design system.
-fc-days-of-week-selector::part(label) { - font-weight: bold; -} +fc-days-of-week-selector::part(label) { + font-weight: var(--lumo-font-weight-bold); +}src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java (3)
41-45: Switch to VerticalLayout and hook shared styles — looks good; also disable default spacingExtending VerticalLayout and adding the "demo" class matches the new shared styles. To avoid double spacing (default VerticalLayout spacing + custom CSS gap), explicitly disable the built-in spacing.
public DayOfWeekSelectorDemo() { - addClassName("demo"); + addClassName("demo"); + setSpacing(false);
62-67: Render value in a deterministic weekday orderSets are unordered; rendering in calendar order avoids flicker and improves readability.
- Consumer<Set<DayOfWeek>> valueRenderer = - value -> valueContent.setText("Value: " + value.stream().map(Enum::name).collect(Collectors.joining(" - "))); + Consumer<Set<DayOfWeek>> valueRenderer = + value -> valueContent.setText("Value: " + value.stream() + .sorted(java.util.Comparator.comparingInt(DayOfWeek::getValue)) + .map(Enum::name) + .collect(Collectors.joining(" - "))));Add this import outside the shown range:
import java.util.Comparator;
65-65: Consider a HorizontalLayout for selector + live value rowPlaces the selector and its live value on a single row (import already present), improving the demo ergonomics.
- add(new Div(selectorOverflow, valueContent)); + add(new HorizontalLayout(selectorOverflow, valueContent));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (7)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java(7 hunks)src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts(1 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java(2 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(1 hunks)
🔇 Additional comments (8)
pom.xml (1)
7-7: Version bump looks goodThe artifact version update to 1.2.0-SNAPSHOT aligns with the new public API surface introduced in this PR.
src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css (1)
54-56: Context menu padding addition LGTMThe padding adjustment for the context-menu list box is scoped and non-invasive.
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (1)
193-194: Recompute overflow when slot content changesCouldn't locate src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts in the repo during verification. If this component (or the component that renders the days slot) contains the , add a slotchange handler so overflow is recalculated when days are added/removed or localized.
- Target file (verify path): src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts — or the component that renders
Suggested change:
- <slot name="daysOfWeek"></slot> + <slot name="daysOfWeek" @slotchange=${this._updateOnOverflow}></slot>Ensure this._updateOnOverflow exists and triggers the overflow recomputation (debounced if appropriate). Please confirm the file path or apply this change in the correct component.
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (2)
255-264: initDefaultDayButtons LGTMSeven buttons initialized with expected labels and mapped days. Works well with the new slotted rendering.
266-286: Lifecycle helpers LGTMThe add/clear/set helpers correctly manage slots and listeners. Good separation of concerns.
src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java (1)
27-28: Shared styles import LGTM
@CssImport("/styles/shared-styles.css")correctly targetssrc/test/resources/META-INF/frontend/styles/shared-styles.cssfor the demo scope.src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java (2)
57-61: Overflow demo usage reads wellGood showcase: constrained width, custom overflow icon via slot, and size customized through the new CSS property.
69-76: Read-only and Disabled examples — good coverageThese standard states are still demonstrated clearly.
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
Show resolved
Hide resolved
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
Show resolved
Hide resolved
src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css
Outdated
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (3)
194-204: Verify: server-side value sync when clicking overflow itemsCalling
originalButton.$server.toggleState()toggles the server-side button, but make sure the field’s server-side value is recomputed too (e.g., CustomField model update). If your Java host doesn’t recompute on child toggle, add a server method (e.g.,requestValueUpdateFromClient) and call it here aftertoggleState().Do you want me to propose the corresponding server-side stub and wire-up?
25-26: Fix: use queryAssignedElements and HTMLElement typing to avoid TS errors and runtime crashesUsing
@queryAssignedNodesyieldsNode[](includes Text nodes) but the code uses element APIs (style,getBoundingClientRect). This can break at runtime (e.g., whitespace becomes a Text node) and fails type-checking. Switch to@queryAssignedElementsand type asHTMLElement[]. Also typeoverflowButtonsaccordingly.Apply:
-import {customElement, property, query, queryAssignedNodes, state} from 'lit/decorators.js'; +import {customElement, property, query, queryAssignedElements, state} from 'lit/decorators.js'; @@ - @queryAssignedNodes({slot: 'daysOfWeek', flatten: true}) - _daysOfWeek!: Array<Node>; + @queryAssignedElements({slot: 'daysOfWeek', flatten: true}) + _daysOfWeek!: Array<HTMLElement>; @@ - const overflowButtons: Node[] = []; + const overflowButtons: HTMLElement[] = []; @@ - for (const button of this._daysOfWeek) { + for (const button of this._daysOfWeek) { button.style.display = ''; const width = button.getBoundingClientRect().width;Also applies to: 34-36, 159-169
189-206: Fix typing and prevent duplicate IDs on cloned overflow items
cloneNodereturns aNode,_originalButtonis not a declared property, and$serverisn’t on theHTMLElementtype. This won’t type-check and can be brittle at runtime. Also, cloning preservesid, which can cause duplicate IDs.Apply:
- private _cloneButton(originalButton: Button) { - const clonedButton = originalButton.cloneNode(true); - clonedButton._originalButton = originalButton; + private _cloneButton(originalButton: HTMLElement) { + const clonedButton = originalButton.cloneNode(true) as HTMLElement & { _originalButton?: HTMLElement }; + clonedButton._originalButton = originalButton; clonedButton.removeAttribute("slot"); + clonedButton.removeAttribute("id"); clonedButton.style.display = ''; clonedButton.addEventListener('click', ev => { // Update server side button component state - clonedButton._originalButton.$server.toggleState(); + (clonedButton._originalButton as any)?.$server?.toggleState?.(); if (clonedButton.getAttribute("theme")) { clonedButton.removeAttribute("theme") } else { clonedButton.setAttribute("theme", "primary"); } // Notify value change to DayOfWeekSelector component this.dispatchEvent(new CustomEvent('change', {bubbles: true})); }); return clonedButton; }
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (2)
216-223: Accessibility: add an aria-label/title to the overflow triggerThe overflow button is icon-only. Provide an accessible name.
- <vaadin-button part="overflow-badge" theme="${this.theme} tertiary-inline" - class="overflow-badge" hidden style="cursor: pointer"> + <vaadin-button part="overflow-badge" theme="${this.theme} tertiary-inline" + class="overflow-badge" hidden style="cursor: pointer" + aria-label="More options" title="More options">If you have an i18n API, consider wiring this to a localized string.
77-95: Nit: consider component-scoped CSS custom property namesThese variables are named
--badge-list-label-*. For clarity and encapsulation, consider namespaced variants like--fc-days-of-week-selector-label-*.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java(7 hunks)src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts(1 hunks)src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
- src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css
- src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T14:30:50.223Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts:195-197
Timestamp: 2025-08-18T14:30:50.223Z
Learning: For Vaadin/LitElement components, when a property like theme can be empty, prefer setting the default value to an empty string (`''`) rather than `null` to avoid rendering "null" in template attribute bindings. This is cleaner than using conditional logic in templates.
Applied to files:
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
🔇 Additional comments (2)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (2)
138-157: Nice: removed magic number with dynamic overflow-button measurementMeasuring the actual overflow trigger width and subtracting it from the container width is robust and theming-safe. Good improvement.
Also applies to: 159-170
41-41: Good call: defaultthemeto empty stringThis avoids “null” leaks in attribute bindings and aligns with prior guidance.
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (4)
228-231: Only set [has-label] when the label is non-emptyThe current condition treats empty string as present, leaving blank label space visible. Switch to a non-empty check.
- if (_changedProperties.has('label')) { - this.toggleAttribute('has-label', this.label != null); - } + if (_changedProperties.has('label')) { + const hasLabel = (this.label ?? '').trim().length > 0; + this.toggleAttribute('has-label', hasLabel); + }
25-25: Use queryAssignedElements instead of queryAssignedNodes to avoid runtime errors with Text nodesThe code treats assigned nodes as HTMLElements (style and getBoundingClientRect), but
@queryAssignedNodesreturnsNode[]. Whitespace becomes Text nodes and will throw at runtime. Switch toqueryAssignedElementsto ensure elements and correct typing.Apply:
-import {customElement, property, query, queryAssignedNodes, state} from 'lit/decorators.js'; +import {customElement, property, query, queryAssignedElements, state} from 'lit/decorators.js';
34-36: Type assigned slotted items as HTMLElement[]Follow-up to the previous comment: ensure the property uses
queryAssignedElementsandHTMLElement[]. This resolves both TS and runtime issues.- @queryAssignedNodes({slot: 'daysOfWeek', flatten: true}) - _daysOfWeek!: Array<Node>; + @queryAssignedElements({slot: 'daysOfWeek', flatten: true}) + _daysOfWeek!: Array<HTMLElement>;
189-206: Fix typing and robustness in _cloneButton; avoid duplicate ids; null-safe server calls
- Parameter type should be
HTMLElementto match the assigned elements.- Cast the clone to include a private
_originalButtonlink.- Remove
idon the clone to prevent duplicate ids.- Use optional chaining on
$server.toggleState()to avoid hard crashes if not present.- private _cloneButton(originalButton: Button) { - const clonedButton = originalButton.cloneNode(true); - clonedButton._originalButton = originalButton; - clonedButton.removeAttribute("slot"); - clonedButton.style.display = ''; - clonedButton.addEventListener('click', ev => { - // Update server side button component state - clonedButton._originalButton.$server.toggleState(); - if (clonedButton.getAttribute("theme")) { - clonedButton.removeAttribute("theme") - } else { - clonedButton.setAttribute("theme", "primary"); - } - // Notify value change to DayOfWeekSelector component - this.dispatchEvent(new CustomEvent('change', {bubbles: true})); - }); - return clonedButton; - } + private _cloneButton(originalButton: HTMLElement) { + const clonedButton = originalButton.cloneNode(true) as HTMLElement & { _originalButton?: HTMLElement }; + clonedButton._originalButton = originalButton; + clonedButton.removeAttribute('slot'); + clonedButton.removeAttribute('id'); // avoid duplicate ids + clonedButton.style.display = ''; + clonedButton.addEventListener('click', () => { + // Update server side button component state + (clonedButton._originalButton as any)?.$server?.toggleState?.(); + // Toggle visual state on the clone + if (clonedButton.getAttribute('theme')) { + clonedButton.removeAttribute('theme'); + } else { + clonedButton.setAttribute('theme', 'primary'); + } + // Notify value change to DayOfWeekSelector component + this.dispatchEvent(new CustomEvent('change', { bubbles: true })); + }); + return clonedButton; + }Note: After this change, the
Buttonimport at Line 26 becomes unused and can be removed for cleanliness.
🧹 Nitpick comments (1)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (1)
213-218: Trigger overflow recalculation on slot changes; add aria-label for overflow button
- Without listening to
slotchange, initial layout may be stale until the first ResizeObserver tick. Hook the slot event to recalc promptly.- Add an accessible name to the overflow trigger.
- <div part="container" class="container" id="container"> - <slot name="daysOfWeek"></slot> - <vaadin-context-menu open-on="click" .items=${this._overflowItems}> - <vaadin-button part="overflow-badge" theme="${this.theme} tertiary-inline" - class="overflow-badge" hidden style="cursor: pointer"> + <div part="container" class="container" id="container"> + <slot name="daysOfWeek" @slotchange=${this._updateOnOverflow}></slot> + <vaadin-context-menu open-on="click" .items=${this._overflowItems}> + <vaadin-button part="overflow-badge" theme="${this.theme} tertiary-inline" + class="overflow-badge" hidden style="cursor: pointer" + aria-label="${this.label ? `${this.label}: more` : 'More'}">Please verify if
vaadin-iconis guaranteed to be registered by your app shell; if not, consider importing it here to ensure the fallback renders:import '@vaadin/icon';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java(7 hunks)src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css(2 hunks)src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts(1 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java(2 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- pom.xml
- src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java
- src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T14:30:50.223Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts:195-197
Timestamp: 2025-08-18T14:30:50.223Z
Learning: For Vaadin/LitElement components, when a property like theme can be empty, prefer setting the default value to an empty string (`''`) rather than `null` to avoid rendering "null" in template attribute bindings. This is cleaner than using conditional logic in templates.
Applied to files:
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
🔇 Additional comments (1)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (1)
40-44: Good: defaulting theme to empty string avoids “null” in attribute bindingsSetting
theme: string = ''aligns with best practice and prevents accidentaltheme="null ..."in the template.
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (1)
139-142: Correct equality semantics for CustomFieldvalueEquals should delegate to Objects.equals. The current check returns false for identical instances, which is surprising and may cause unnecessary value change handling.
Apply:
- @Override - protected boolean valueEquals(Set<DayOfWeek> value1, Set<DayOfWeek> value2) { - return value1 != value2 && Objects.equals(value1, value2); - } + @Override + protected boolean valueEquals(Set<DayOfWeek> value1, Set<DayOfWeek> value2) { + return Objects.equals(value1, value2); + }
♻️ Duplicate comments (5)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (4)
159-187: Avoid false positives: don’t reserve overflow button width unless overflow actually occurs (two-pass)Always subtracting the overflow badge width can push one item into overflow needlessly. Measure once without reserving; if overflow occurs, redo with reservation.
Apply:
- private _updateOnOverflow() { - const containerWidth = this.getBoundingClientRect().width; - let usedWidth = 0; - const overflowButtons: Node[] = []; - const overflowButtonWidth = this._getOverflowButtonWidth(); - - for (const button of this._daysOfWeek) { - button.style.display = ''; - const width = button.getBoundingClientRect().width; - - if (usedWidth + width > containerWidth - overflowButtonWidth) { - const clonedButton = this._cloneButton(button); - overflowButtons.push(clonedButton); - - // hide button from slot - button.style.display = 'none'; - } else { - usedWidth += width; - } - } - - // Update context menu items from hidden buttons - this._overflowItems = overflowButtons.map(button => ({ - component: button, - })); - - // Update badges visibility - this._overflowBadge.toggleAttribute('hidden', this._overflowItems.length < 1); - } + private _updateOnOverflow() { + const containerWidth = this.getBoundingClientRect().width; + const allButtons = this._daysOfWeek; + + // Reset visibility before measuring + for (const btn of allButtons) { + btn.style.display = ''; + } + + const measure = (reserveOverflow: boolean) => { + const overflowWidth = reserveOverflow ? this._getOverflowButtonWidth() : 0; + let used = 0; + const overflow: HTMLElement[] = []; + for (const btn of allButtons) { + const w = btn.getBoundingClientRect().width; + if (used + w > containerWidth - overflowWidth) { + overflow.push(this._cloneButton(btn)); + btn.style.display = 'none'; + } else { + used += w; + } + } + return overflow; + }; + + // First pass without reserving badge space + let overflowButtons = measure(false); + if (overflowButtons.length > 0) { + // Second pass reserving badge space + for (const btn of allButtons) { + btn.style.display = ''; + } + overflowButtons = measure(true); + } + + this._overflowItems = overflowButtons.map((button) => ({ component: button })); + this._overflowBadge.toggleAttribute('hidden', this._overflowItems.length < 1); + }
34-36: Fix typing: assigned nodes are treated as HTMLElements; switch to queryAssignedElementsYou access style and getBoundingClientRect on items from _daysOfWeek, but it’s typed as Node[]. This is a TS/type safety and potential runtime issue. Use queryAssignedElements and HTMLElement[].
Apply:
-import {customElement, property, query, queryAssignedNodes, state} from 'lit/decorators.js'; +import {customElement, property, query, queryAssignedElements, state} from 'lit/decorators.js'; @@ - @queryAssignedNodes({slot: 'daysOfWeek', flatten: true}) - _daysOfWeek!: Array<Node>; + @queryAssignedElements({slot: 'daysOfWeek'}) + _daysOfWeek!: Array<HTMLElement>;Also applies to: 25-25, 165-175
228-231: Fix has-label for empty/whitespace labelsthis.label != null treats '' as present. Only set has-label when non-empty after trimming.
Apply:
- if (_changedProperties.has('label')) { - this.toggleAttribute('has-label', this.label != null); - } + if (_changedProperties.has('label')) { + const hasLabel = (this.label ?? '').trim().length > 0; + this.toggleAttribute('has-label', hasLabel); + }
189-206: Critical: overflow menu changes don’t update server-side value; fix typing and invoke host value updateClicking a cloned item calls originalButton.$server.toggleState(), but does not call updateValue() on the field, so the CustomField model value stays stale. Also, the clone typing relies on ad-hoc properties on Node and will fail TS checks.
Apply:
- private _cloneButton(originalButton: Button) { - const clonedButton = originalButton.cloneNode(true); - clonedButton._originalButton = originalButton; + private _cloneButton(originalButton: HTMLElement) { + const clonedButton = originalButton.cloneNode(true) as HTMLElement & { _originalButton?: HTMLElement }; + clonedButton._originalButton = originalButton; clonedButton.removeAttribute("slot"); + clonedButton.removeAttribute("id"); clonedButton.style.display = ''; clonedButton.addEventListener('click', ev => { // Update server side button component state - clonedButton._originalButton.$server.toggleState(); + (clonedButton._originalButton as any).$server.toggleState(); + // Ensure the server-side field value is recomputed + (this as any).$server.requestValueUpdateFromClient(); if (clonedButton.getAttribute("theme")) { clonedButton.removeAttribute("theme") } else { clonedButton.setAttribute("theme", "primary"); } // Notify value change to DayOfWeekSelector component this.dispatchEvent(new CustomEvent('change', {bubbles: true})); }); return clonedButton; }Note: This requires a new @ClientCallable method on the Java host to recompute the value (see Java review).
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (1)
302-302: Add host-level client-callable to recompute model value (needed by overflow menu actions)When an overflow clone triggers originalButton.$server.toggleState() from the client, no server-side click event fires and updateValue() isn’t invoked. Provide a callable the TS component can invoke after toggling.
Apply:
@@ -} + @ClientCallable + private void requestValueUpdateFromClient() { + updateValue(); + } +}
🧹 Nitpick comments (3)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (2)
37-39: Remove unused query _container_container is queried but never used. Drop it to avoid dead code.
Apply:
- @query('[part~="container"]') - _container!: HTMLElement; + // (removed unused _container)
214-214: Trigger overflow recalculation when slotted content changesOverflow should be recalculated on slot content changes, not only on resize. Add a slotchange handler.
Apply:
- <slot name="daysOfWeek"></slot> + <slot name="daysOfWeek" @slotchange=${this._updateOnOverflow}></slot>src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (1)
255-268: LGTM: overflow icon setter removes previous slotted iconRemoving prior overflowIcon before appending avoids duplicates; class name assignment is fine. Consider extracting the "overflowIcon" slot name into a private constant for reuse.
Optional:
private static final String OVERFLOW_ICON_SLOT = "overflowIcon";Then replace string literals accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java(7 hunks)src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css(1 hunks)src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts(1 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java(2 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- pom.xml
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css
- src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T12:15:44.547Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java:155-160
Timestamp: 2025-08-18T12:15:44.547Z
Learning: Vaadin's Element API has an overloaded setAttribute(String attribute, boolean value) method that automatically handles boolean-to-attribute conversion: when value is true, it sets the attribute to an empty string (""); when value is false, it removes the attribute entirely via removeAttribute(). This means code like getElement().setAttribute("readonly", readOnly) is correct in Vaadin applications.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
📚 Learning: 2025-08-18T14:30:50.223Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts:195-197
Timestamp: 2025-08-18T14:30:50.223Z
Learning: For Vaadin/LitElement components, when a property like theme can be empty, prefer setting the default value to an empty string (`''`) rather than `null` to avoid rendering "null" in template attribute bindings. This is cleaner than using conditional logic in templates.
Applied to files:
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
🔇 Additional comments (5)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (3)
31-33: LGTM: correct element typing for overflow badgeUsing HTMLElement for _overflowBadge matches the queried and avoids incorrect narrowing.
143-157: LGTM: robust overflow badge width measurementTemporarily unhiding to measure and restoring hidden state removes the magic number and adapts to theme/icon changes.
40-45: LGTM: theme default avoids “null” leakage in attributeDefaulting theme and label to '' aligns with past guidance and prevents rendering "null" in theme binding.
Also applies to: 216-221
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (2)
156-168: LGTM: readOnly property/attribute handling and button enablementUsing Element#setAttribute(String, boolean) is correct in Vaadin and the per-button enable/disable plus CSS class toggling is consistent.
238-246: LGTM: first-day-of-week reordering logicRotation after sorting by DayOfWeek is correct and handles Sunday-first via modulo math.
paodb
left a comment
paodb
left a comment
There was a problem hiding this comment.
The implementation LGTM. And I tested and it looks good. I just left a minor comment on the demo, and there are also some CodeRabbit suggestions that need to be addressed.
| import com.vaadin.flow.component.button.Button; | ||
| import com.vaadin.flow.component.html.Div; | ||
| import com.vaadin.flow.component.orderedlayout.FlexComponent; | ||
| import com.vaadin.flow.component.orderedlayout.HorizontalLayout; |
There was a problem hiding this comment.
The imports for FlexComponent & HorizontalLayout are unused imports. Please remove them.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (1)
140-142: valueEquals logic is inverted for identical references; return Objects.equals directlyReturning
value1 != value2 && Objects.equals(value1, value2)means identical instances are considered “not equal” (false), which can break value change detection. UseObjects.equals.Apply:
- @Override - protected boolean valueEquals(Set<DayOfWeek> value1, Set<DayOfWeek> value2) { - return value1 != value2 && Objects.equals(value1, value2); - } + @Override + protected boolean valueEquals(Set<DayOfWeek> value1, Set<DayOfWeek> value2) { + return Objects.equals(value1, value2); + }
♻️ Duplicate comments (4)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (4)
159-187: Prevent false positives: use two-pass measurement so overflow button is only reserved when neededAlways reserving the overflow trigger width can cause an unnecessary overflow entry when all buttons would otherwise fit.
Apply:
- private _updateOnOverflow() { - const containerWidth = this.getBoundingClientRect().width; - let usedWidth = 0; - const overflowButtons: Node[] = []; - const overflowButtonWidth = this._getOverflowButtonWidth(); - - for (const button of this._daysOfWeek) { - button.style.display = ''; - const width = button.getBoundingClientRect().width; - - if (usedWidth + width > containerWidth - overflowButtonWidth) { - const clonedButton = this._cloneButton(button); - overflowButtons.push(clonedButton); - - // hide button from slot - button.style.display = 'none'; - } else { - usedWidth += width; - } - } - - // Update context menu items from hidden buttons - this._overflowItems = overflowButtons.map(button => ({ - component: button, - })); - - // Update badges visibility - this._overflowBadge.toggleAttribute('hidden', this._overflowItems.length < 1); - } + private _updateOnOverflow() { + const containerWidth = this.getBoundingClientRect().width; + const allButtons = this._daysOfWeek; + // Reset visibility before measuring + for (const btn of allButtons) btn.style.display = ''; + const measure = (reserveOverflow: boolean) => { + const overflowWidth = reserveOverflow ? this._getOverflowButtonWidth() : 0; + let used = 0; + const overflow: HTMLElement[] = []; + for (const btn of allButtons) { + const w = btn.getBoundingClientRect().width; + if (used + w > containerWidth - overflowWidth) { + overflow.push(this._cloneButton(btn)); + btn.style.display = 'none'; + } else { + used += w; + } + } + return overflow; + }; + // First pass: assume no overflow badge + let overflowButtons = measure(false); + if (overflowButtons.length > 0) { + // Second pass: reserve space for the overflow badge + for (const btn of allButtons) btn.style.display = ''; + overflowButtons = measure(true); + } + this._overflowItems = overflowButtons.map((button) => ({ component: button })); + this._overflowBadge.toggleAttribute('hidden', this._overflowItems.length < 1); + }
228-231: has-label should only reflect non-empty labels
this.label != nulltoggles the attribute even for''. Use a non-empty check.Apply:
- if (_changedProperties.has('label')) { - this.toggleAttribute('has-label', this.label != null); - } + if (_changedProperties.has('label')) { + const hasLabel = (this.label ?? '').trim().length > 0; + this.toggleAttribute('has-label', hasLabel); + }
25-25: Type errors: using Node where HTMLElement is required; switch to queryAssignedElements and fix typingsYou access element APIs (
style,getBoundingClientRect) and pass items to_cloneButton(originalButton: Button), but_daysOfWeekis typed asNode[]from@queryAssignedNodes. This will fail type-checking and risks runtime issues. Prefer@queryAssignedElementsand type asHTMLElement[]. Also adjust the import.Apply:
-import {customElement, property, query, queryAssignedNodes, state} from 'lit/decorators.js'; +import {customElement, property, query, queryAssignedElements, state} from 'lit/decorators.js'; @@ - @queryAssignedNodes({slot: 'daysOfWeek', flatten: true}) - _daysOfWeek!: Array<Node>; + @queryAssignedElements({slot: 'daysOfWeek'}) + _daysOfWeek!: Array<HTMLElement>; @@ - for (const button of this._daysOfWeek) { + for (const button of this._daysOfWeek) { button.style.display = ''; const width = button.getBoundingClientRect().width;Also applies to: 34-36, 165-168
189-206: Overflow clone click doesn’t update server-side field value; fix typing and invoke host updaterClicking an overflow clone calls
$server.toggleState()on the original button but never callsupdateValue()on the host, so the CustomField model value isn’t recomputed server-side. Typings are also too narrow (Node) and rely on an undeclared_originalButtonproperty.Apply:
- private _cloneButton(originalButton: Button) { - const clonedButton = originalButton.cloneNode(true); - clonedButton._originalButton = originalButton; + private _cloneButton(originalButton: HTMLElement) { + const clonedButton = originalButton.cloneNode(true) as HTMLElement & { _originalButton?: HTMLElement }; + clonedButton._originalButton = originalButton; clonedButton.removeAttribute("slot"); + clonedButton.removeAttribute("id"); clonedButton.style.display = ''; clonedButton.addEventListener('click', ev => { // Update server side button component state - clonedButton._originalButton.$server.toggleState(); + (clonedButton._originalButton as any).$server.toggleState(); + // Ensure the server-side field value is recomputed + (this as any).$server.requestValueUpdateFromClient?.(); if (clonedButton.getAttribute("theme")) { clonedButton.removeAttribute("theme") } else { clonedButton.setAttribute("theme", "primary"); } // Notify value change to DayOfWeekSelector component this.dispatchEvent(new CustomEvent('change', {bubbles: true})); }); return clonedButton; }Follow-up required in Java host (see inline comment on DayOfWeekSelector.java) to add
@ClientCallable requestValueUpdateFromClient().
🧹 Nitpick comments (2)
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (2)
201-210: Validate i18n lists length to avoid IndexOutOfBoundsBoth setters assume 7 entries. Add a precondition check to fail fast with a clear message.
Apply:
public void setWeekDaysShort(List<String> weekdaysShort) { Objects.requireNonNull(weekdaysShort); + if (weekdaysShort.size() != 7) { + throw new IllegalArgumentException("weekdaysShort must contain 7 items (sun..sat)"); + } @@ public void setWeekDaysTooltip(List<String> weekdaysTooltip) { Objects.requireNonNull(weekdaysTooltip); + if (weekdaysTooltip.size() != 7) { + throw new IllegalArgumentException("weekdaysTooltip must contain 7 items (sun..sat)"); + }Also applies to: 217-226
237-247: Simplify reordering logic using rotation on an already-sorted listCurrent approach sorts then manual-indexes. You can simplify and reduce allocation by rotating a sorted list view.
Apply:
- DayOfWeekButton[] buttons = dayButtons.toArray(DayOfWeekButton[]::new); - if (buttons[0].dayOfWeek != first) { - var sortedButtons = new ArrayList<DayOfWeekButton>(); - Arrays.sort(buttons, Comparator.comparing(DayOfWeekButton::getDayOfWeek)); - for (int i = 0; i < buttons.length; i++) { - sortedButtons.add(buttons[(i + first.getValue() - 1) % buttons.length]); - } - setDayButtons(sortedButtons); - } + var sorted = new ArrayList<>(dayButtons); + sorted.sort(Comparator.comparing(DayOfWeekButton::getDayOfWeek)); + if (!sorted.isEmpty() && sorted.get(0).dayOfWeek != first) { + int offset = first.getValue() - 1; // 0..6 + Collections.rotate(sorted, -offset); + } + setDayButtons(sorted);Note: Add
import java.util.Collections;if not present.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
pom.xml(1 hunks)src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java(7 hunks)src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css(1 hunks)src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts(1 hunks)src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java(1 hunks)src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java(2 hunks)src/test/resources/META-INF/frontend/styles/shared-styles.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/resources/META-INF/frontend/styles/shared-styles.css
- src/main/resources/META-INF/frontend/styles/fc-days-of-week-selector-styles.css
- src/test/java/com/flowingcode/vaadin/addons/DemoLayout.java
- src/test/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelectorDemo.java
- pom.xml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T14:30:50.252Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts:195-197
Timestamp: 2025-08-18T14:30:50.252Z
Learning: For Vaadin/LitElement components, when a property like theme can be empty, prefer setting the default value to an empty string (`''`) rather than `null` to avoid rendering "null" in template attribute bindings. This is cleaner than using conditional logic in templates.
Applied to files:
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
📚 Learning: 2025-08-18T12:15:44.614Z
Learnt from: flang
PR: FlowingCode/DayOfWeekSelectorAddon#14
File: src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java:155-160
Timestamp: 2025-08-18T12:15:44.614Z
Learning: Vaadin's Element API has an overloaded setAttribute(String attribute, boolean value) method that automatically handles boolean-to-attribute conversion: when value is true, it sets the attribute to an empty string (""); when value is false, it removes the attribute entirely via removeAttribute(). This means code like getElement().setAttribute("readonly", readOnly) is correct in Vaadin applications.
Applied to files:
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
🔇 Additional comments (4)
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts (2)
40-45: Good: theme/label default to empty string to avoid “null” renderingUsing
''defaults prevents leaking literal “null” into thethemeattribute. This aligns with the prior learning for Lit/Vaadin components.
139-157: Measuring overflow button width when hidden is handled correctlyTemporarily unhiding to measure actual width avoids magic numbers and adapts to theme/icon changes. Solid approach.
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java (2)
156-168: Good: correct boolean attribute handling for readonly and per-button stateUsing
getElement().setAttribute("readonly", readOnly)leverages Vaadin’s boolean overload to add/remove the attribute. Disabling buttons and adding a readOnly class per button is consistent with styling.
249-268: Overflow icon replacement logic looks goodRemoving any existing slotted icon before appending the new one prevents accumulation and keeps DOM clean. Nice touch adding a public CssProperties constant for the size variable.
src/main/java/com/flowingcode/vaadin/addons/dayofweekselector/DayOfWeekSelector.java
Show resolved
Hide resolved
src/main/resources/META-INF/resources/frontend/src/fc-days-of-week-selector.ts
Show resolved
Hide resolved
github-project-automation
bot
moved this from In Progress
to Pending release
in Flowing Code Addons
Default overflow button:
Overflow button icon is customizable as well:
Summary by CodeRabbit