Skip to content

fix: dropdowns should update the month based on their index #2742

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gpbl
Copy link
Owner

@gpbl gpbl commented Apr 16, 2025

No description provided.

@gpbl gpbl requested a review from Copilot April 16, 2025 13:31
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the dropdown behavior by adjusting the month calculations based on a provided display index.

  • Updates the month and year change handlers in DayPicker to use an extra displayIndex parameter
  • Introduces a new example and corresponding tests to showcase the dropdown caption layout behavior

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/DayPicker.tsx Modified handleMonthChange and handleYearChange to incorporate display index offsets for computing the new month
examples/index.ts Added export for the MultipleMonthsWithDropdown example
examples/MultipleMonthsWithDropdown.tsx Created a new example demonstrating dropdown usage
examples/MultipleMonthsWithDropdown.test.tsx Added tests for verifying month and year updates from the dropdown selection

},
(date: Date, displayIndex: number) =>
(e: ChangeEvent<HTMLSelectElement>) => {
const selectedMonth = Number(e.target.value);
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Subtracting displayIndex from selectedMonth directly can be prone to off-by-one errors if the dropdown indices do not align exactly with the month values. Please add an inline comment explaining the rationale and ensuring that the arithmetic correctly transforms the select input value to the intended month.

Suggested change
const selectedMonth = Number(e.target.value);
const selectedMonth = Number(e.target.value);
// Subtracting `displayIndex` from `selectedMonth` ensures that the selected dropdown value
// corresponds to the correct month. This assumes that `selectedMonth` is zero-based (0 = January)
// and `displayIndex` aligns with the dropdown's offset for the displayed months.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

(e: ChangeEvent<HTMLSelectElement>) => {
const selectedYear = Number(e.target.value);
let month = dateLib.setYear(dateLib.startOfMonth(date), selectedYear);
month = dateLib.addMonths(month, displayIndex * -1);
Copy link
Preview

Copilot AI Apr 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using displayIndex multiplied by -1 to adjust the month when setting the year may introduce errors if displayIndex is not consistently defined. Consider adding documentation or refactoring to clarify the expected behavior and arithmetic justification.

Suggested change
month = dateLib.addMonths(month, displayIndex * -1);
// Adjust the month based on the display index. A negative displayIndex moves the month backward.
month = dateLib.addMonths(month, -displayIndex);

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant