Added Calendar component and used for filtering#27657
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA Calendar UI component and CalendarDayButton were added (with date-fns and react-day-picker deps), exported from the shade components barrel, and documented with Storybook stories (Default and DatePicker). The filters feature gained parse/format helpers and a FilterDatePicker component that replaces native date inputs for single date and daterange filters. Unit tests and a small posts comment-fields formatting tweak were also added. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/shade/src/components/ui/filters.tsx`:
- Around line 671-676: FilterDatePickerProps/FilterDatePicker currently takes
value/onChange but never calls field.onInputChange, breaking consumers that rely
on FilterFieldConfig.onInputChange for committed edits; update
FilterDatePickerProps to accept field?: FilterFieldConfig and inside the
FilterDatePicker component invoke field.onInputChange(newValue) at the same
point you call onChange (or when the date selection is committed) so date and
daterange flows behave like other input branches; make sure to reference the
existing field parameter and call its onInputChange method with the string value
used by onChange.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5387ef4d-8f6f-468b-9c39-1db4ec5a08d6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/posts/src/views/comments/components/comments-filters.tsxapps/shade/package.jsonapps/shade/src/components.tsapps/shade/src/components/ui/calendar.stories.tsxapps/shade/src/components/ui/calendar.tsxapps/shade/src/components/ui/filters.tsx
💤 Files with no reviewable changes (1)
- apps/posts/src/views/comments/components/comments-filters.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27657 +/- ##
==========================================
+ Coverage 73.18% 73.69% +0.51%
==========================================
Files 1561 1512 -49
Lines 127215 125820 -1395
Branches 15391 15129 -262
==========================================
- Hits 93103 92724 -379
+ Misses 33133 32150 -983
+ Partials 979 946 -33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
weylandswart
force-pushed
the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
from
877e674 to
cea3c8a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/shade/src/components/features/filters/filters.tsx (1)
671-676:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
field.onInputChangestill not invoked for date / daterange.
FilterDatePickerPropsdoesn't includeonInputChange, andhandleInputBlur/handleSelectonly call the localonChange. Every other input-like branch (time,datetime,text/number,email/url/tel) still forwardsfield.onInputChange, so committed edits ondate/daterangefilters silently stop reaching consumers that listen to that callback throughFilterFieldConfig. Same applies to both daterange call sites (1888-1893, 1900-1905) and the single-date site (1975-1980).🛠️ Suggested wiring
interface FilterDatePickerProps<T = unknown> { field?: FilterFieldConfig<T>; value: string; onChange: (value: string) => void; + onInputChange?: (e: React.ChangeEvent<HTMLInputElement>) => void; className?: string; } @@ -function FilterDatePicker<T = unknown>({ - field, - value, - onChange, - className -}: FilterDatePickerProps<T>) { +function FilterDatePicker<T = unknown>({ + field, + value, + onChange, + onInputChange, + className +}: FilterDatePickerProps<T>) { @@ const handleInputBlur = (e: React.FocusEvent<HTMLInputElement>) => { if (e.target.value !== value) { onChange(e.target.value); + onInputChange?.(e as unknown as React.ChangeEvent<HTMLInputElement>); } }; @@ const handleSelect = (date: Date | undefined) => { if (!date) { setLocalValue(''); onChange(''); + onInputChange?.({target: {value: ''}} as React.ChangeEvent<HTMLInputElement>); return; } const formatted = formatFilterDateValue(date); setMonth(date); setLocalValue(formatted); onChange(formatted); + onInputChange?.({target: {value: formatted}} as React.ChangeEvent<HTMLInputElement>); setOpen(false); };And forward at the three usage sites, e.g.:
<FilterDatePicker className={field.className} field={field} value={(values[0] as string) || ''} onChange={v => onChange([v] as T[])} + onInputChange={field.onInputChange} />Also applies to: 716-737, 1888-1905, 1975-1980
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/features/filters/filters.tsx` around lines 671 - 676, FilterDatePickerProps is missing onInputChange so committed edits for date/daterange never call field.onInputChange; add onInputChange?: (value: string) => void to FilterDatePickerProps (or accept it via field?.onInputChange) and update the handlers (handleInputBlur and handleSelect inside the date/daterange picker components) to call field.onInputChange(value) in addition to the local onChange, mirroring how other input branches do it; ensure the three usage sites that render the daterange and single-date pickers forward the field.onInputChange through props (or rely on field) so consumers of FilterFieldConfig receive the callback.
🧹 Nitpick comments (1)
apps/shade/src/components/ui/calendar.stories.tsx (1)
53-73: ⚡ Quick winConsider expanding story coverage to mirror shadcn's calendar examples.
Only
Default(single) and a popover composition are demonstrated. The shadcn calendar docs cover several common variants worth showing here so consumers see the supported props and states at a glance:mode="range",mode="multiple",captionLayout="dropdown"(already used byFilterDatePicker), and adisabledexample. As per coding guidelines: "Add a sibling *.stories.tsx file for new UI components with ... stories showing different use cases/variants (sizes, states, important props)" and "For ShadCN components, copy the examples from the ShadCN component documentation".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/ui/calendar.stories.tsx` around lines 53 - 73, Add additional Story exports to mirror shadcn calendar variants: create Range, Multiple, CaptionDropdown, and Disabled stories alongside Default and DatePicker by reusing or creating example components (e.g., SingleCalendarExample -> RangeCalendarExample/MultipleCalendarExample or render SingleCalendarExample with props mode="range" and mode="multiple"), add a CaptionDropdown story demonstrating captionLayout="dropdown" (see existing FilterDatePicker for usage), and add Disabled story showing the calendar with disabled prop; follow shadcn examples for markup/props and include descriptive docs.description.story for each export.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@apps/shade/src/components/features/filters/filters.tsx`:
- Around line 671-676: FilterDatePickerProps is missing onInputChange so
committed edits for date/daterange never call field.onInputChange; add
onInputChange?: (value: string) => void to FilterDatePickerProps (or accept it
via field?.onInputChange) and update the handlers (handleInputBlur and
handleSelect inside the date/daterange picker components) to call
field.onInputChange(value) in addition to the local onChange, mirroring how
other input branches do it; ensure the three usage sites that render the
daterange and single-date pickers forward the field.onInputChange through props
(or rely on field) so consumers of FilterFieldConfig receive the callback.
---
Nitpick comments:
In `@apps/shade/src/components/ui/calendar.stories.tsx`:
- Around line 53-73: Add additional Story exports to mirror shadcn calendar
variants: create Range, Multiple, CaptionDropdown, and Disabled stories
alongside Default and DatePicker by reusing or creating example components
(e.g., SingleCalendarExample -> RangeCalendarExample/MultipleCalendarExample or
render SingleCalendarExample with props mode="range" and mode="multiple"), add a
CaptionDropdown story demonstrating captionLayout="dropdown" (see existing
FilterDatePicker for usage), and add Disabled story showing the calendar with
disabled prop; follow shadcn examples for markup/props and include descriptive
docs.description.story for each export.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d1cf1ca7-9969-4eea-bd1e-3465de6f04fa
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/posts/src/views/comments/comment-fields.tsapps/shade/package.jsonapps/shade/src/components.tsapps/shade/src/components/features/filters/filters.tsxapps/shade/src/components/ui/calendar.stories.tsxapps/shade/src/components/ui/calendar.tsx
weylandswart
force-pushed
the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
from
cea3c8a to
62dc518
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (6)
apps/shade/src/components/features/filters/filters.tsx (4)
649-659: 💤 Low value
parseFilterDateValueaccepts overflow dates.
new Date(year, month - 1, day)silently normalizes out-of-range parts, so'2024-02-30'parses to Mar 1 and'2024-13-01'to Jan 2025. Native<input type="date">won't typically produce such values, but consumers can call this with arbitrary strings (URL roundtrip, programmatic init). Consider validating the round-trip:🛡️ Proposed validation
const parseFilterDateValue = (value: string): Date | undefined => { if (!value) { return undefined; } const parts = value.split('-').map(Number); if (parts.length !== 3 || parts.some(n => Number.isNaN(n))) { return undefined; } const [year, month, day] = parts; - return new Date(year, month - 1, day); + const date = new Date(year, month - 1, day); + if ( + date.getFullYear() !== year || + date.getMonth() !== month - 1 || + date.getDate() !== day + ) { + return undefined; + } + return date; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/features/filters/filters.tsx` around lines 649 - 659, parseFilterDateValue currently lets overflow dates silently normalize (e.g., "2024-02-30" -> Mar 1); after parsing parts and constructing the Date, validate that the resulting Date's year, month and day match the original components (compare d.getFullYear() === year, d.getMonth() + 1 === month, d.getDate() === day) and return undefined if any mismatch; update the function (parseFilterDateValue) to perform this round-trip check so only exact, non-overflow dates are accepted.
1888-1905: 🏗️ Heavy liftDaterange: start/end share the same
Calendarpopover with no cross-field constraints.Both pickers render an independent single-date calendar; nothing prevents picking
endDate < startDate, and there's no visual range highlight (whichreact-day-picker'smode="range"would give for free). Consider migrating to a singleCalendarwithmode="range"for daterange, or at minimum passdisabled={{before: startDate}}to the end picker anddisabled={{after: endDate}}to the start picker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/features/filters/filters.tsx` around lines 1888 - 1905, The two FilterDatePicker instances (rendering startDate and endDate) currently each open independent single-date calendars so users can pick end < start and there's no range highlight; update the implementation to either (preferred) switch to a single Calendar with mode="range" wired to the component's onChange to produce [startDate,endDate], or (minimal) pass cross-field disable rules to the existing pickers—i.e., give the end FilterDatePicker a disabled prop like {before: startDate} and the start FilterDatePicker a disabled prop like {after: endDate} so selections are constrained; ensure onChange still emits [startDate,endDate] via the existing onChange handler.
691-714: 💤 Low valueStale calendar selection when
valueis cleared externally.When the parent clears the filter (e.g. "Clear filters" or operator switch),
valuebecomes'',parsedbecomesundefined, and theif (parsed) setMonth(parsed)guard at line 703 leavesmonthpointing at the previously viewed month. That's usually fine, but combine it withselected={parsed}(line 787) beingundefinedwhile the popover is open, and the calendar shows no selection in a possibly-unrelated month. Consider also resetting the state in the cleared branch or usingsetMonth(parsed ?? new Date())to recenter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/features/filters/filters.tsx` around lines 691 - 714, The month state isn't cleared when value becomes '' because the effect only sets month when parsed is truthy; update the useEffect that currently reads if (parsed) setMonth(parsed) to handle the cleared case by calling setMonth(parsed ?? new Date()) (or setMonth(undefined) if you prefer to collapse to no selection) so month is recentered/reset when parsed is undefined; this involves modifying the effect that uses parsed and setMonth (the parsed variable from parseFilterDateValue, the month state and setMonth setter) to set the fallback state when parsed is falsy.
757-766: 💤 Low valueThe native
type="date"display format is controlled by OS/browser locale, not app-settable user preference.
<input type="date">always stores values as YYYY-MM-DD per spec but displays the date mask in the browser's locale (e.g., MM/DD/YYYY for en-US, DD/MM/YYYY for en-GB). There is no HTML attribute or API to override this, and Safari/WebKit historically show rendering inconsistencies. Since the popover Calendar is the primary interaction surface (the input's picker UI is hidden), the text input format inconsistency is mitigated. Considertype="text"with explicit format hints andparseFilterDateValueif explicit format control becomes a requirement, but the current approach is pragmatic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/features/filters/filters.tsx` around lines 757 - 766, The input uses type="date" which forces browser/OS-local display formatting; change it to type="text" to control visible format and parsing: update the JSX input in filters.tsx (the element using inputRef, localValue, handleInputBlur, handleInputChange) to type="text", add a visible format hint/placeholer, and modify handleInputChange/handleInputBlur to parse/validate using the existing parseFilterDateValue utility (or central parse function) and continue to store values as YYYY-MM-DD for internal state; ensure the popover calendar remains the primary picker and keep hiding native pickers CSS as-is.apps/shade/src/components/ui/calendar.stories.tsx (1)
53-73: 💤 Low valueConsider adding stories for range/multiple modes and
captionLayout="dropdown".The Calendar supports
mode="range",mode="multiple", andcaptionLayout="dropdown"(whichFilterDatePickeractually uses). Adding stories for those would document the variants consumers will hit and align with the guideline to "demonstrate key variants and states".As per coding guidelines: "Add a sibling
*.stories.tsxfile for new UI components with an overview (what/why) and stories showing different use cases/variants (sizes, states, important props)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/src/components/ui/calendar.stories.tsx` around lines 53 - 73, Add additional Story exports demonstrating Calendar variants: create new stories (e.g., Range and Multiple) alongside Default and DatePicker that render examples like RangeCalendarExample and MultipleCalendarExample using Calendar props mode="range" and mode="multiple", and add a story (e.g., CaptionDropdown) showing captionLayout="dropdown" (match FilterDatePicker usage) — ensure each story has docs.description.story and a descriptive name so consumers can see these key variants and states.apps/shade/package.json (1)
130-134: 💤 Low valueConsider dropping
date-fnsas an unused dependency.
date-fns@4.1.0is listed in package.json but is not imported or used anywhere in the codebase.react-day-picker@9.14.0works without it as an optional peer dependency. Bothcalendar.tsxand theFilterDatePickercomponent infilters.tsxuse nativeDateAPIs and hand-rolled formatting helpers instead. Removing this dependency would reduce bundle size with no functional impact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/shade/package.json` around lines 130 - 134, The package.json currently lists date-fns@4.1.0 but the codebase (e.g., calendar.tsx and the FilterDatePicker component in filters.tsx) does not import or use it; remove the unused dependency by deleting the "date-fns" entry from package.json, run your package manager to update the lockfile (npm/yarn/pnpm install), ensure there are no stray imports of "date-fns" anywhere (search the repo), and run tests/build to verify nothing breaks (react-day-picker remains as the optional peer dependency).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/shade/package.json`:
- Around line 130-134: The package.json currently lists date-fns@4.1.0 but the
codebase (e.g., calendar.tsx and the FilterDatePicker component in filters.tsx)
does not import or use it; remove the unused dependency by deleting the
"date-fns" entry from package.json, run your package manager to update the
lockfile (npm/yarn/pnpm install), ensure there are no stray imports of
"date-fns" anywhere (search the repo), and run tests/build to verify nothing
breaks (react-day-picker remains as the optional peer dependency).
In `@apps/shade/src/components/features/filters/filters.tsx`:
- Around line 649-659: parseFilterDateValue currently lets overflow dates
silently normalize (e.g., "2024-02-30" -> Mar 1); after parsing parts and
constructing the Date, validate that the resulting Date's year, month and day
match the original components (compare d.getFullYear() === year, d.getMonth() +
1 === month, d.getDate() === day) and return undefined if any mismatch; update
the function (parseFilterDateValue) to perform this round-trip check so only
exact, non-overflow dates are accepted.
- Around line 1888-1905: The two FilterDatePicker instances (rendering startDate
and endDate) currently each open independent single-date calendars so users can
pick end < start and there's no range highlight; update the implementation to
either (preferred) switch to a single Calendar with mode="range" wired to the
component's onChange to produce [startDate,endDate], or (minimal) pass
cross-field disable rules to the existing pickers—i.e., give the end
FilterDatePicker a disabled prop like {before: startDate} and the start
FilterDatePicker a disabled prop like {after: endDate} so selections are
constrained; ensure onChange still emits [startDate,endDate] via the existing
onChange handler.
- Around line 691-714: The month state isn't cleared when value becomes ''
because the effect only sets month when parsed is truthy; update the useEffect
that currently reads if (parsed) setMonth(parsed) to handle the cleared case by
calling setMonth(parsed ?? new Date()) (or setMonth(undefined) if you prefer to
collapse to no selection) so month is recentered/reset when parsed is undefined;
this involves modifying the effect that uses parsed and setMonth (the parsed
variable from parseFilterDateValue, the month state and setMonth setter) to set
the fallback state when parsed is falsy.
- Around line 757-766: The input uses type="date" which forces browser/OS-local
display formatting; change it to type="text" to control visible format and
parsing: update the JSX input in filters.tsx (the element using inputRef,
localValue, handleInputBlur, handleInputChange) to type="text", add a visible
format hint/placeholer, and modify handleInputChange/handleInputBlur to
parse/validate using the existing parseFilterDateValue utility (or central parse
function) and continue to store values as YYYY-MM-DD for internal state; ensure
the popover calendar remains the primary picker and keep hiding native pickers
CSS as-is.
In `@apps/shade/src/components/ui/calendar.stories.tsx`:
- Around line 53-73: Add additional Story exports demonstrating Calendar
variants: create new stories (e.g., Range and Multiple) alongside Default and
DatePicker that render examples like RangeCalendarExample and
MultipleCalendarExample using Calendar props mode="range" and mode="multiple",
and add a story (e.g., CaptionDropdown) showing captionLayout="dropdown" (match
FilterDatePicker usage) — ensure each story has docs.description.story and a
descriptive name so consumers can see these key variants and states.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1115b8bf-a30e-4bb0-880c-7cabbd753883
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/posts/src/views/comments/comment-fields.tsapps/shade/package.jsonapps/shade/src/components.tsapps/shade/src/components/features/filters/filters.tsxapps/shade/src/components/ui/calendar.stories.tsxapps/shade/src/components/ui/calendar.tsx
| ); | ||
| } | ||
|
|
||
| if (field.type === 'daterange') { |
There was a problem hiding this comment.
@weylandswart should we expose this somewhere? I remember us discussing in the mob that we're lacking date ranges, but it seems the filter UI supports it.
There was a problem hiding this comment.
We probably should? I feel like it'd be nice to do between as a filter operator. I'd add a follow up issue for that, though. Purely to get the better looking calendar shipped as priority numero uno.
jonatansberg
force-pushed
the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
from
62dc518 to
1990f64
Compare
Ref https://linear.app/ghost/issue/BER-3598/date-picker-ux-issues-in-member-filtering Adding a Shade calendar component and replacing the existing native datepicker for filtering with it.
jonatansberg
force-pushed
the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
from
1990f64 to
f6ba57a
Compare
jonatansberg
left a comment
jonatansberg
left a comment
There was a problem hiding this comment.
@weylandswart I pushed an updated version that fixed some reliability things. With those fixes, I think it looks good enough code wise. The date input is a bit wide though. Maybe we want to make it a bit more narrow?
|
@jonatansberg Agreed. Fixed in 7e38eef. |
no issue Safari still opened its native date picker from date input text clicks, so the filter date control now uses an editable text field paired with the Shade calendar popover while keeping calendar selections synced into the input.
kevinansfield
force-pushed
the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
from
3dbf079 to
059db79
Compare
kevinansfield
deleted the
weyland-ber-3598-date-picker-ux-issues-in-member-filtering
branch
3 participants
Ref https://linear.app/ghost/issue/BER-3598/date-picker-ux-issues-in-member-filtering
Adding a Shade calendar component and replacing the existing native datepicker for filtering with it.