feat: shared UI components and Cally date pickers#140
Merged
Conversation
…ally Replace native <input type="date"> and <input type="month"> with custom calendar components powered by the Cally web component library, styled by daisyUI's .cally class. New shared components in CoreComponents: - date_range_picker: Cally calendar-range in a dropdown (sm/default sizes) - date_picker: Cally calendar-date in a dropdown for single dates - month_range_picker: custom month grid with year nav for billing periods - table_container: bordered scrollable table wrapper (9 usages) - empty_state: centered "no results" message (5 usages) - search_input: magnifying glass icon + text input (2 usages) - reset_filters_button: filter clear button (3 usages) - outline-destructive button variant (6 usages) Migrated all LiveView templates to use shared components, eliminating ~100 lines of duplicated HTML. Added ADR 0043 and CLAUDE.md guidelines for component-first development. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a component-driven UI: ADR and docs, CoreComponents (table container, empty state, search/reset, date/month pickers, button variant), a vendored Cally calendar web-component, LiveSocket hooks for calendar popovers, CSS overrides for calendar styling, and many LiveView templates updated to use the new components. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Trigger as Trigger/Popover
participant Cally as CallyCalendar
participant Hook as LiveView Hook
participant Inputs as Hidden Form Inputs
User->>Trigger: Click trigger
Trigger->>Trigger: Toggle popover (show calendar)
User->>Cally: Select date(s) or month(s)
Cally->>Hook: Emit "change" event with value
Hook->>Hook: Parse value (single / range / month-range)
Hook->>Inputs: Populate hidden inputs (data-from / data-to / data-value)
Hook->>Inputs: Dispatch bubbling "input" event(s)
Hook->>Trigger: Hide popover
Note over Hook: destroyed(): remove outside-click listener
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
assets/js/app.js (1)
210-219: Consider dispatching input event on both hidden inputs.Currently only
fromInputdispatches theinputevent. While this works for triggering a singlephx-change, if form validation or processing depends on individual field change events, thetoInputchange may be missed.💡 Optional: dispatch on both inputs
const DateRangePicker = calendarPickerHook("calendar-range", (hook, value) => { if (value && value.includes("/")) { const [from, to] = value.split("/") const fromInput = hook.el.querySelector("[data-from]") const toInput = hook.el.querySelector("[data-to]") fromInput.value = from toInput.value = to fromInput.dispatchEvent(new Event("input", { bubbles: true })) + toInput.dispatchEvent(new Event("input", { bubbles: true })) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/app.js` around lines 210 - 219, The DateRangePicker callback currently parses value into from/to and only dispatches an "input" Event on fromInput; update the handler in the calendarPickerHook callback (DateRangePicker) to also dispatch an "input" Event on toInput after setting its value so both hidden inputs emit input events (fromInput and toInput) to ensure any per-field listeners or phx-change behavior run for the "to" field as well.lib/ksef_hub_web/components/core_components.ex (3)
717-729: Consider adding@restfor HTML attribute forwarding.Similar to
table_container, this component would benefit from@restsupport for attributes likedata-testid.♻️ Add `@rest` support
attr :icon, :string, default: nil attr :class, :string, default: nil + attr :rest, :global slot :inner_block, required: true `@spec` empty_state(map()) :: Phoenix.LiveView.Rendered.t() def empty_state(assigns) do ~H""" - <div class={["text-center text-muted-foreground py-8", `@class`]}> + <div class={["text-center text-muted-foreground py-8", `@class`]} {`@rest`}> <.icon :if={`@icon`} name={`@icon`} class="size-8 mx-auto mb-2 opacity-40" /> <p>{render_slot(`@inner_block`)}</p> </div> """ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/components/core_components.ex` around lines 717 - 729, The empty_state component lacks HTML attribute forwarding; add a global rest attr and pass it into the container div so callers can supply arbitrary HTML attributes (e.g., data-testid). Concretely, add attr :rest, :global to the component attrs (alongside :icon and :class) and include {`@rest`} on the root <div> in empty_state (similar to how table_container handles `@rest`) so attributes are forwarded to the rendered element.
695-707: Consider adding@restfor HTML attribute forwarding.The
table_containercomponent follows the required conventions (@doc,@spec,attr,slot), but lacks@restfor forwarding arbitrary HTML attributes likedata-testid. As per coding guidelines, components should use@rest(:global) to forward HTML attributes.♻️ Add `@rest` support
attr :class, :string, default: nil + attr :rest, :global slot :inner_block, required: true `@spec` table_container(map()) :: Phoenix.LiveView.Rendered.t() def table_container(assigns) do ~H""" - <div class={["rounded-lg border border-border overflow-hidden", `@class`]}> + <div class={["rounded-lg border border-border overflow-hidden", `@class`]} {`@rest`}> <div class="overflow-x-auto"> {render_slot(`@inner_block`)} </div> </div> """ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/components/core_components.ex` around lines 695 - 707, The component table_container is missing HTML attribute forwarding; add an attr :rest, :global to the component (alongside the existing attr :class and slot :inner_block) and spread `@rest` onto the root container element inside the HEEx template (e.g., include {...@rest} on the outer <div> that currently renders class={["rounded-lg border border-border overflow-hidden", `@class`]} ) so arbitrary attributes like data-testid are forwarded.
1264-1269: Add defensive handling for malformed input informat_month_label.The
String.split(ym, "-")pattern match assumes exactly two parts. Malformed input could cause aMatchError. Since this is called with output fromdate_to_monthwhich validates format, the risk is low, but defensive coding would be safer.🛡️ Defensive pattern match
`@spec` format_month_label(String.t()) :: String.t() defp format_month_label(ym) when is_binary(ym) do - [year, month_str] = String.split(ym, "-") - {month_num, _} = Integer.parse(month_str) - "#{Enum.at(`@months`, month_num - 1)} #{year}" + case String.split(ym, "-") do + [year, month_str] -> + {month_num, _} = Integer.parse(month_str) + "#{Enum.at(`@months`, month_num - 1)} #{year}" + + _ -> + ym + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/components/core_components.ex` around lines 1264 - 1269, format_month_label currently assumes ym is a binary "YYYY-MM" and will crash on malformed input; update format_month_label to defensively handle bad input by using String.split(ym, "-", parts: 2) and pattern-matching the result (e.g., case [year, month_str] -> parse month with Integer.parse(month_str) and if successful return "#{Enum.at(`@months`, month_num - 1)} #{year}" else fall back to returning ym or a safe default; for non-binary inputs add a clause or guard to return a safe default), ensuring you reference format_month_label and `@months` in the change.assets/vendor/cally.js (1)
1-31: Add version and license header to vendored library.The vendored
cally.jsfile lacks version information and license attribution. This makes it difficult to track updates and ensure license compliance. Consider adding a header comment with version, source URL, and license.📝 Suggested header
+/** + * Cally - Calendar Web Components + * `@version` [VERSION] + * `@source` https://github.com/WickyNilliams/cally + * `@license` MIT + */ class le {What is the current version of the Cally calendar library by WickyNilliams?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/vendor/cally.js` around lines 1 - 31, Add a header comment to the top of assets/vendor/cally.js (the vendored library that defines class le with its current/getter/setter and on method) containing the Cally library version, source URL (upstream repo or npm page), copyright/author attribution (WickyNilliams), and the license text or an SPDX identifier (e.g., MIT) so license compliance and update tracking are clear; place the header as a block comment above the class definition and include a brief "Vendored from" note and a link to the original source.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@assets/js/app.js`:
- Around line 210-219: The DateRangePicker callback currently parses value into
from/to and only dispatches an "input" Event on fromInput; update the handler in
the calendarPickerHook callback (DateRangePicker) to also dispatch an "input"
Event on toInput after setting its value so both hidden inputs emit input events
(fromInput and toInput) to ensure any per-field listeners or phx-change behavior
run for the "to" field as well.
In `@assets/vendor/cally.js`:
- Around line 1-31: Add a header comment to the top of assets/vendor/cally.js
(the vendored library that defines class le with its current/getter/setter and
on method) containing the Cally library version, source URL (upstream repo or
npm page), copyright/author attribution (WickyNilliams), and the license text or
an SPDX identifier (e.g., MIT) so license compliance and update tracking are
clear; place the header as a block comment above the class definition and
include a brief "Vendored from" note and a link to the original source.
In `@lib/ksef_hub_web/components/core_components.ex`:
- Around line 717-729: The empty_state component lacks HTML attribute
forwarding; add a global rest attr and pass it into the container div so callers
can supply arbitrary HTML attributes (e.g., data-testid). Concretely, add attr
:rest, :global to the component attrs (alongside :icon and :class) and include
{`@rest`} on the root <div> in empty_state (similar to how table_container handles
`@rest`) so attributes are forwarded to the rendered element.
- Around line 695-707: The component table_container is missing HTML attribute
forwarding; add an attr :rest, :global to the component (alongside the existing
attr :class and slot :inner_block) and spread `@rest` onto the root container
element inside the HEEx template (e.g., include {...@rest} on the outer <div>
that currently renders class={["rounded-lg border border-border
overflow-hidden", `@class`]} ) so arbitrary attributes like data-testid are
forwarded.
- Around line 1264-1269: format_month_label currently assumes ym is a binary
"YYYY-MM" and will crash on malformed input; update format_month_label to
defensively handle bad input by using String.split(ym, "-", parts: 2) and
pattern-matching the result (e.g., case [year, month_str] -> parse month with
Integer.parse(month_str) and if successful return "#{Enum.at(`@months`, month_num
- 1)} #{year}" else fall back to returning ym or a safe default; for non-binary
inputs add a clause or guard to return a safe default), ensuring you reference
format_month_label and `@months` in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2606a66c-10fe-4448-bba8-3dd9ed1062f0
📒 Files selected for processing (20)
CLAUDE.mdassets/css/app.cssassets/js/app.jsassets/vendor/cally.jsdocs/adr/0043-component-driven-ui.mdlib/ksef_hub_web/components/core_components.exlib/ksef_hub_web/live/bank_account_live.exlib/ksef_hub_web/live/category_live/index.exlib/ksef_hub_web/live/company_live/form.exlib/ksef_hub_web/live/company_live/index.exlib/ksef_hub_web/live/dashboard_live.exlib/ksef_hub_web/live/export_live/index.exlib/ksef_hub_web/live/invoice_live/index.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/payment_request_live/index.exlib/ksef_hub_web/live/sync_live.exlib/ksef_hub_web/live/team_live.exlib/ksef_hub_web/live/team_member_live/show.exlib/ksef_hub_web/live/token_live.extest/ksef_hub_web/live/invoice_live/show_test.exs
- Dispatch input event on both from and to hidden inputs in DateRangePicker so per-field listeners fire for both - Add vendor header to cally.js with version, license, source URL - Add attr :rest, :global to table_container and empty_state for HTML attribute forwarding (e.g. data-testid) - Make format_month_label defensive against malformed input Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
assets/js/app.js (2)
240-240: Specify radix inparseIntcall.Omitting the radix can lead to unexpected parsing behavior in edge cases. Explicitly passing
10is a best practice.♻️ Proposed fix
- this.year = parseInt(this.yearEl.textContent) + this.year = parseInt(this.yearEl.textContent, 10)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/app.js` at line 240, The parseInt call that sets this.year from this.yearEl.textContent should explicitly specify the radix to avoid ambiguous parsing; update the expression in the constructor/initializer where this.year = parseInt(this.yearEl.textContent) to pass 10 as the second argument (e.g., parseInt(..., 10)) so it reliably parses decimal years.
181-184: Consider adding null checks for queried elements.If the component markup doesn't render expected selectors (
[data-trigger],[data-popover], or the calendar element), the code will throw when callingaddEventListeneron null. Adding defensive checks or early returns would improve robustness.🛡️ Suggested defensive check
mounted() { this.calendar = this.el.querySelector(calendarSelector) this.trigger = this.el.querySelector("[data-trigger]") this.popover = this.el.querySelector("[data-popover]") + + if (!this.calendar || !this.trigger || !this.popover) { + console.warn(`${calendarSelector} picker: missing required elements`) + return + } this.trigger.addEventListener("click", (e) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@assets/js/app.js` around lines 181 - 184, The mounted() method currently assigns this.calendar, this.trigger, and this.popover via this.el.querySelector without null checks; update mounted() to defensively verify each queried element (using calendarSelector and selectors "[data-trigger]" and "[data-popover]") and either early-return or skip wiring event listeners when any of those are null so calls like addEventListener never run on null; ensure subsequent code that references this.calendar/this.trigger/this.popover checks existence or is only executed after these guards.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@assets/js/app.js`:
- Around line 279-287: The MonthRangePicker commit block sets
this.fromInput.value and this.toInput.value but only dispatches an "input" event
for this.fromInput, which can stop per-field listeners on the to input from
running; update the commit sequence in the MonthRangePicker handler (the same
area that calls this.highlightMonths(), this.updateLabel(), and
this.popover.classList.add("hidden")) to also dispatch new Event("input", {
bubbles: true }) on this.toInput after setting its value so both hidden inputs
fire their input listeners.
---
Nitpick comments:
In `@assets/js/app.js`:
- Line 240: The parseInt call that sets this.year from this.yearEl.textContent
should explicitly specify the radix to avoid ambiguous parsing; update the
expression in the constructor/initializer where this.year =
parseInt(this.yearEl.textContent) to pass 10 as the second argument (e.g.,
parseInt(..., 10)) so it reliably parses decimal years.
- Around line 181-184: The mounted() method currently assigns this.calendar,
this.trigger, and this.popover via this.el.querySelector without null checks;
update mounted() to defensively verify each queried element (using
calendarSelector and selectors "[data-trigger]" and "[data-popover]") and either
early-return or skip wiring event listeners when any of those are null so calls
like addEventListener never run on null; ensure subsequent code that references
this.calendar/this.trigger/this.popover checks existence or is only executed
after these guards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f3a9d76-df9b-4a05-842c-b5603918cbb5
📒 Files selected for processing (3)
assets/js/app.jsassets/vendor/cally.jslib/ksef_hub_web/components/core_components.ex
Also add explicit radix to parseInt for year parsing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<input type="date">and<input type="month">with custom calendar components powered by the Cally web component library, styled automatically by daisyUI's.callyclassNew shared components
date_range_picker<input type="date">on 4 pagesdate_picker<input type="date">on invoice edit formmonth_range_picker<input type="month">on billing periodtable_containerempty_statesearch_inputreset_filters_buttonoutline-destructivePages updated
Dashboard, Invoice list, Invoice show, Export, Payment requests, Categories, Tokens, Bank accounts, Sync, Team, Company list, Company form
Test plan
mix compile --warnings-as-errors— cleanmix test— 1855 tests pass, 0 failuresmix format --check-formatted— cleanmix credo --strict— no issues🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Styling
Documentation