fix:#559 Improve UX calendars and popovers CreateGroup and EditGroup #654
Conversation
…', update unit tests
tonyb650
requested review from
a team and
shashilo
and removed request for
a team
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
shashilo
requested review from
a team and
shashilo
and removed request for
a team and
shashilo
There was a problem hiding this comment.
Pull Request Overview
This PR improves the UX for calendar and popover interactions in the gift exchange group creation and editing flows. The changes fix calendar size inconsistency across months and implement auto-closing behavior for popover inputs when selections are made.
Key Changes:
- Added
fixedWeeksprop to the Calendar component to maintain consistent calendar height across all months - Implemented controlled popover state management for Budget, Drawing Date, and Exchange Date inputs to enable auto-closing on selection
- Extracted budget options to a shared constants file (
BUDGET_OPTIONS) to reduce code duplication
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
jest.setup.js |
Minor formatting fix (added blank line) |
constants/exchangeGroupOptions.ts |
New file containing shared budget options array |
components/Calendar/calendar.tsx |
Added fixedWeeks prop to maintain consistent calendar height |
components/Calendar/calendar.test.tsx |
Added test for fixed weeks behavior (has implementation bug) |
app/gift-exchanges/[id]/edit/page.tsx |
Refactored to use shared budget options and added controlled popover state (has critical React Hooks violation) |
app/gift-exchanges/[id]/edit/page.test.tsx |
Added tests for popover auto-close behavior and improved test organization |
app/create-group-page/page.tsx |
Refactored to use shared budget options and added controlled popover state (has critical React Hooks violation and date validation inconsistency) |
app/create-group-page/page.test.tsx |
Added tests for popover auto-close behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| className="h-9" | ||
| /> | ||
| <CommandList> | ||
| <CommandEmpty>No framework found.</CommandEmpty> |
There was a problem hiding this comment.
The error message in CommandEmpty is misleading for the budget selection context. "No framework found." should be updated to something more appropriate like "No budget options found." or "No price range found."
| <CommandEmpty>No framework found.</CommandEmpty> | |
| <CommandEmpty>No budget options found.</CommandEmpty> |
|
This would be for a separate ticket, but did you spot any major differences in the forms used in |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes refactor budget selection across group creation and editing pages to use a centralized BUDGET_OPTIONS constant, replace hard-coded price ranges with controlled Popover components for budget and date selection, add the fixedWeeks prop to Calendar for consistent sizing, and expand test coverage for these UI patterns. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Page
participant Form as Form State
participant Popover as Popover/Calendar
User->>UI: Click budget/date button
UI->>Popover: Open popover (set open state)
Popover-->>User: Display options/calendar
User->>Popover: Select option/date
Popover->>Form: Update form value
Popover->>UI: Close popover (set open state false)
UI-->>User: Display selected value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
♻️ Duplicate comments (2)
app/create-group-page/page.tsx (1)
182-252: Budget popover implementation is clean; adjustCommandEmptycopyThe budget field now:
- Uses a controlled
PopoverwithbudgetOpen/setBudgetOpen.- Renders options from centralized
BUDGET_OPTIONS.- Closes the popover on selection via
setBudgetOpen(false).- Shows the selected label or a fallback “Select a price range”.
This satisfies the “close on selection” UX requirement and de-duplicates price ranges.
One small nit:
CommandEmptystill says “No framework found.”, which is misleading in this context. Consider updating the copy, e.g.:- <CommandList> - <CommandEmpty>No framework found.</CommandEmpty> + <CommandList> + <CommandEmpty>No budget options found.</CommandEmpty>components/Calendar/calendar.test.tsx (1)
6-19: AvoidforEachwith async user-event; convert test to async +forloopThe test intent is solid, but
Array.from({ length: 12 }).forEach(async () => { ... })inside a non‑asyncitmeans Jest won’t wait for the async clicks/expectations. This can lead to flaky or misleading results.Consider rewriting as an
asynctest with a simple loop:- it('displays the same number of weeks/rows for every month of the year', () => { + it('displays the same number of weeks/rows for every month of the year', async () => { render(<Calendar initialFocus />); - const user = userEvent.setup(); + const user = userEvent.setup(); const daysDisplayedCount = screen.getAllByRole('gridcell').length; expect(daysDisplayedCount).toBeGreaterThan(31); - Array.from({ length: 12 }).forEach(async () => { - const nextMonthButton = screen.getByLabelText('Go to next month'); - await user.click(nextMonthButton); - - expect(screen.getAllByRole('gridcell')).toHaveLength(daysDisplayedCount); - }); + for (let i = 0; i < 12; i++) { + const nextMonthButton = screen.getByLabelText('Go to next month'); + await user.click(nextMonthButton); + expect(screen.getAllByRole('gridcell')).toHaveLength(daysDisplayedCount); + } });
🧹 Nitpick comments (8)
components/Calendar/calendar.tsx (1)
31-31: Hard-codingfixedWeeksmakes calendar height consistent for all usagesAdding
fixedWeeksachieves the UX goal of a stable calendar grid across months for every consumer ofCalendar. If you ever need a non‑fixed calendar elsewhere, you’ll have to adjust this component or add a prop to makefixedWeeksconfigurable, but for this PR’s objective the change looks correct.app/create-group-page/page.test.tsx (2)
10-12: StrengthenuseRoutermock to be closer to Next’s APIRight now
useRouterreturns{}, which works because these tests don’t hitrouter.push, but it’s brittle if future tests exercise form submission.Consider returning an object with the methods the page uses, e.g.:
-jest.mock('next/navigation', () => ({ - useRouter: () => {}, -})); +jest.mock('next/navigation', () => ({ + useRouter: () => ({ + push: jest.fn(), + prefetch: jest.fn(), + }), +}));This keeps tests closer to runtime behavior and avoids surprises if the component starts calling router methods during render or submit.
89-147: Popover and calendar interaction tests are solid; tweakscrollIntoViewmockingThese new tests nicely exercise the budget and date popover behavior and align with the new
data-testidhooks andBUDGET_OPTIONSusage. One small improvement: instead of assigning directly toElement.prototype.scrollIntoView, use a spy sojest.restoreAllMocks()actually restores it:describe('Budget input', () => { - beforeEach(() => { - Element.prototype.scrollIntoView = jest.fn(); - }); + beforeEach(() => { + jest + .spyOn(Element.prototype, 'scrollIntoView') + .mockImplementation(() => {}); + }); afterEach(() => { jest.restoreAllMocks(); });This avoids leaving a mutated prototype behind and makes the intent clearer.
app/create-group-page/page.tsx (1)
310-360: GuardgiftDrawingDatewhen computing disabled dates for exchange calendarThe exchange date field correctly:
- Uses a controlled
PopoverwithexchangeDateOpen.- Writes via
field.onChangeand closes on selection.- Attempts to enforce
exchange_date > drawing_datewithdisabled={[{ before: addDays(new Date(giftDrawingDate), 1) }]}.However, when
giftDrawingDateis still unset,new Date(giftDrawingDate)will be an invalid date, which then gets passed intoaddDays. While most libraries tolerate this by propagating an “Invalid Date”, it’s safer and clearer to guard:- <Calendar - mode="single" - selected={field.value} - onSelect={(inputValue) => { - field.onChange(inputValue); - setExchangeDateOpen(false); - }} - disabled={[ - { before: addDays(new Date(giftDrawingDate), 1) }, - ]} - initialFocus - /> + <Calendar + mode="single" + selected={field.value} + onSelect={(inputValue) => { + field.onChange(inputValue); + setExchangeDateOpen(false); + }} + disabled={ + giftDrawingDate + ? [{ before: addDays(giftDrawingDate, 1) }] + : undefined + } + initialFocus + />This keeps the intent explicit and avoids passing invalid dates into the calendar library.
app/gift-exchanges/[id]/edit/page.tsx (2)
239-312: Budget popover integrates well; consider leveragingfield.onChange/setValueoptionsThe budget popover wiring correctly:
- Shows the selected label via
BUDGET_OPTIONS.- Updates the form’s
budgetvalue.- Closes on selection via
setBudgetOpen(false).One small refinement:
form.setValue('budget', budgetOption.value)does not mark the field as touched/dirty or trigger validation by default in react-hook-form. If you want inline validation and dirtiness tracking to behave like a regular input, consider:- onSelect={() => { - form.setValue('budget', budgetOption.value); - setBudgetOpen(false); - }} + onSelect={() => { + field.onChange(budgetOption.value); + // or, if you prefer setValue: + // form.setValue('budget', budgetOption.value, { + // shouldDirty: true, + // shouldTouch: true, + // shouldValidate: true, + // }); + setBudgetOpen(false); +}}This keeps UX/validation behavior more consistent with other fields.
368-420: Guardexchange_datedisabled boundary when drawing date is unsetThe intent to constrain
exchange_datevia:disabled={[{ before: addDays(new Date(giftDrawingDate), 1) }]}lines up with the schema rule
exchange_date > drawing_date, but there are two edge cases:
- When
giftDrawingDateis initiallyundefined(before the user picks a drawing date),new Date(giftDrawingDate)produces an invalid Date, which can lead to unclear behavior in the calendar’s disabled matcher.- If users open the exchange-date picker before setting
drawing_date, they can currently pick any date; the refine will catch it on submit, but the UI won’t guide them.A small guard makes the behavior clearer and avoids relying on an invalid Date:
- disabled={[ - { before: addDays(new Date(giftDrawingDate), 1) }, - ]} + disabled={ + giftDrawingDate + ? [{ before: addDays(giftDrawingDate, 1) }] + : [] // or e.g. [{ before: addDays(new Date(), 1) }] if you want at least "tomorrow" + }This keeps the disabled logic well-defined and tightly aligned with the validation rule.
app/gift-exchanges/[id]/edit/page.test.tsx (2)
18-25: Type-safeResizeObservermockThe
MockResizeObserverclass is fine for JSDOM, but assigning it directly:global.ResizeObserver = MockResizeObserver;can be rejected by TypeScript in stricter setups because the constructor signature may not match the DOM
ResizeObservertype.To avoid type friction, it’s safer to either cast or align the constructor signature, e.g.:
-class MockResizeObserver { +class MockResizeObserver { observe() {} unobserve() {} disconnect() {} } -global.ResizeObserver = MockResizeObserver; +global.ResizeObserver = MockResizeObserver as unknown as typeof ResizeObserver;This keeps runtime behavior the same while satisfying TS.
84-133: Popover-close interaction tests align with UX goals (minor naming nit)The budget, drawing-date, and exchange-date tests:
- Render
EditGroupPagewith mocks in place.- Open each popover via
data-testidtriggers.- Assert the correct number of options / grid cells.
- Click an option/day and assert that options or
gridcells disappear, confirming close-on-select behavior.This matches the PR’s UX objective and should catch regressions.
Minor readability nit: in the “Exchange date calendar input” test, the trigger is stored in
drawingDateTriggerButton:const drawingDateTriggerButton = screen.getByTestId( 'exchange-date-button', );Renaming this variable to
exchangeDateTriggerButtonwould better reflect its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/create-group-page/page.test.tsx(2 hunks)app/create-group-page/page.tsx(4 hunks)app/gift-exchanges/[id]/edit/page.test.tsx(1 hunks)app/gift-exchanges/[id]/edit/page.tsx(4 hunks)components/Calendar/calendar.test.tsx(1 hunks)components/Calendar/calendar.tsx(1 hunks)constants/exchangeGroupOptions.ts(1 hunks)jest.setup.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/gift-exchanges/[id]/edit/page.test.tsx (2)
app/gift-exchanges/[id]/edit/page.tsx (1)
EditGroupPage(72-440)constants/exchangeGroupOptions.ts (1)
BUDGET_OPTIONS(4-14)
app/create-group-page/page.test.tsx (2)
app/create-group-page/page.tsx (1)
CreateGroupPage(70-374)constants/exchangeGroupOptions.ts (1)
BUDGET_OPTIONS(4-14)
app/gift-exchanges/[id]/edit/page.tsx (2)
lib/utils.ts (1)
cn(13-15)constants/exchangeGroupOptions.ts (1)
BUDGET_OPTIONS(4-14)
app/create-group-page/page.tsx (2)
lib/utils.ts (1)
cn(13-15)constants/exchangeGroupOptions.ts (1)
BUDGET_OPTIONS(4-14)
🔇 Additional comments (8)
jest.setup.js (1)
4-4: Jest DOM setup looks goodThe
@testing-library/jest-domimport is appropriate for the RTL tests in this PR; no changes needed.constants/exchangeGroupOptions.ts (1)
4-14: CentralizedBUDGET_OPTIONSconstant is clear and reusableExtracting the budget ranges into a shared
BUDGET_OPTIONSarray withas constimproves consistency between the create/edit pages and tests; this looks good as-is.app/create-group-page/page.tsx (2)
42-76: Top-level popover state and budget validation align with UX goalsImporting
useState/BUDGET_OPTIONSand introducingbudgetOpen,drawingDateOpen, andexchangeDateOpenat the component level fixes the previous “hooks inside render” issue and gives you precise control over when each popover opens/closes. Updating the budgetrequired_errormessage to “Please select a budget.” also matches the current field semantics.
255-305: Drawing date calendar popover behavior matches the requirementsFor the drawing date field, the controlled
PopoverplusCalendar:
- Disables past dates via
disabled={[{ before: new Date() }]}.- Writes the selected date into the form with
field.onChange.- Closes the popover on selection with
setDrawingDateOpen(false).- Provides a clear formatted label or “Pick a date” placeholder.
This is a good fit for both the UX objective (closing on selection) and preventing invalid past drawing dates.
app/gift-exchanges/[id]/edit/page.tsx (3)
46-66: Schema + centralized budget options look solidImporting
BUDGET_OPTIONSand using a clearerrequired_errorplus thegroup_imagerefine keeps validation messages consistent and expresses constraints explicitly. No issues from a correctness perspective.
72-80: Good fix for popover open state (no hooks-in-render issues anymore)Defining
budgetOpen,drawingDateOpen, andexchangeDateOpenat the top level and wiring them into the popovers resolves the earlier “hooks inside render” concern while making the open/close behavior explicit and predictable.Also applies to: 173-173
315-365: Drawing date calendar popover behavior matches requirementsThe drawing date field:
- Uses controlled
drawingDateOpenstate.- Disables past dates via
disabled={[{ before: new Date() }]}.- Closes the popover on date selection (
setDrawingDateOpen(false)).- Feeds the selected date through
field.onChangefor RHF/zod.This aligns well with the UX goal of closing the date picker after selection and preventing past dates.
app/gift-exchanges/[id]/edit/page.test.tsx (1)
50-82: Calendar disabled-date test provides good coverageThe
Calendar componentsuite with fixedcurrentDate, fake timers, and checks for past/today/tomorrow being disabled/enabled exercises the coredisabledmatcher logic well and stays decoupled from the page component. This is a solid unit-style test for the calendar itself.
Description
Before (2 Issues):
Issue 1
Calendar size was inconsistent. As the user changes the month displayed, the number of weeks visible would change depending on the particular month.
Issue 2
Choosing an input from 'popover' style input fields would not trigger the closure of the popover in question. This applied to the "Budget", "Drawing Date", and "Exchange Date" inputs.
After:
These fixes were applied to both the
CreateGrouppage and theEditGrouppage.Issue 1
Calendar size was made consistent by applying the
fixedweeksattribute to the<DayPicker/>within the<Calendar/>component.Issue 2
For each input field, a state variable was added to track the
openstate of the associated popover. For each field, theonSelectscript now includes an update to 'setOpen(false)` to close the popover.Closes #559
Additional information
The array of Price Ranges / Budget Options was extracted to a separate file to reduce repetition.
Screenshots
Before, number of weeks not 'fixed':
After, number of weeks 'fixed':
Pre-submission checklist
test #001: created unit test for __ component)Peer Code ReviewersandSenior+ Code Reviewersgroupsgis-code-questionsSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.