fix(date-picker): month and year picker scroller #3789
Conversation
Conforming to name convention.
Removing clsx.
🦋 Changeset detectedLatest commit: 214385f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@buchananwill is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces updates to three packages: Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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
Outside diff range and nitpick comments (1)
packages/components/calendar/src/use-calendar-picker.ts (1)
100-102: Avoid variable shadowing by renaming local variabledate.The local variable
datedeclared in line 100 shadows thedateprop from the outer scope. This could lead to confusion and potential bugs. Consider renaming the local variable to improve code clarity.Apply this diff to rename the variable:
-let date = state.focusedDate.set(list === "months" ? {month: value} : {year: value}); +let updatedDate = state.focusedDate.set(list === "months" ? {month: value} : {year: value}); -state.setFocusedDate(date); +state.setFocusedDate(updatedDate);
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- .changeset/serious-snails-count.md (1 hunks)
- packages/components/accordion/src/accordion-tree.tsx (1 hunks)
- packages/components/calendar/src/use-calendar-picker.ts (3 hunks)
Additional comments not posted (2)
.changeset/serious-snails-count.md (1)
1-6: LGTM!The changeset file follows the correct format and provides a clear description of the changes made to the packages. The version updates are appropriate based on the nature of the changes:
@nextui-org/accordion: Minor version update for new backward-compatible functionality.@nextui-org/calendar: Patch version update for backward-compatible bug fixes.The description also provides a high-level overview of the fix related to the UI and data synchronization for the month/year picker component.
packages/components/calendar/src/use-calendar-picker.ts (1)
6-6: LGTM!The import statement correctly adds
useCallbackfrom "react", which is appropriately used in the code.
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (3)
.changeset/sixty-weeks-help.md (1)
1-7: LGTM! Consider a minor clarification in the description.The changeset looks good and correctly identifies the packages being updated. The patch version bumps are appropriate for the described change.
Consider slightly expanding the description for clarity:
-Added an aria and test label for the picker toggle. +Added an aria label for accessibility and a test label for the picker toggle to improve testability.This minor change would provide more context about the purpose of each label.
packages/components/calendar/src/calendar-header.tsx (2)
104-104: LGTM: Excellent accessibility improvementThe addition of the
aria-labelattribute using themonthAndYearPickerToggleconstant significantly enhances the accessibility of the calendar component. This change allows screen readers to provide clear context for the button's purpose.Consider moving the
aria-labelattribute closer to other core Button props for better readability. For example:<Button {...headerProps} - aria-label={monthAndYearPickerToggle} disableAnimation={disableAnimation} endContent={<ChevronDownIcon className="chevron-icon" />} + aria-label={monthAndYearPickerToggle} onKeyDown={handleKeyDown} {...buttonPickerProps} >This is a minor suggestion and doesn't affect functionality.
Verify alignment with PR objectives
The changes in
calendar-header.tsximprove accessibility, which is commendable. However, they do not address the synchronization issue between the month and year in the date picker UI, as mentioned in the PR objectives.Based on the search results, there are no other changes in this PR related to "datepicker synchronization," "UI rectangle intersection," or updating data directly from the value attribute. Therefore, the PR does not fully meet its stated goals.
Analysis chain
Line range hint
1-114: Verify alignment with PR objectivesThe changes in this file improve accessibility, which is commendable. However, they don't seem to directly address the synchronization issue between the month and year in the date picker UI, as mentioned in the PR objectives.
Could you please clarify how these changes relate to fixing the "UI rectangle intersection error" and ensuring that "the data is updated directly from the value attribute"? If these issues are addressed in other files, it would be helpful to include those changes in the PR for a comprehensive review.
To assist in verifying the PR objectives, you can run the following script to search for related changes:
This will help identify any other relevant changes that might be part of this PR but weren't included in the current review.
To better verify the PR objectives, please run the following scripts to search within
.tsand.tsxfiles for relevant changes:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for changes related to date picker synchronization and UI rectangle intersection # Test: Search for files with changes related to date picker, month, year, or synchronization rg --type typescript -i '(datepicker|month|year|sync|intersection)' # Test: Search for files with changes related to updating from value attribute rg --type typescript -i '(value.*attribute|attribute.*value)'Length of output: 203
Script:
#!/bin/bash # Description: Search for changes related to date picker synchronization and UI rectangle intersection within TypeScript files. # Search for files with .ts and .tsx extensions and look for relevant keywords fd --extension ts --extension tsx | xargs rg -i '(datepicker|month|year|sync|intersection)' # Search for files updating from the value attribute within TypeScript files fd --extension ts --extension tsx | xargs rg -i '(value.*attribute|attribute.*value)'Length of output: 122139
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- .changeset/sixty-weeks-help.md (1 hunks)
- packages/components/calendar/src/calendar-header.tsx (2 hunks)
- packages/components/date-picker/tests/date-picker.test.tsx (3 hunks)
Additional comments not posted (4)
packages/components/calendar/src/calendar-header.tsx (1)
21-22: LGTM: Good addition of a descriptive constantThe addition of the
monthAndYearPickerToggleconstant is a good practice. It provides a clear, reusable string for the aria-label, which enhances accessibility and maintainability.packages/components/date-picker/__tests__/date-picker.test.tsx (3)
8-8: New import added for month and year picker toggleThe addition of this import suggests that new tests for the month and year picker functionality will be implemented. This is a good practice to ensure new features are properly tested.
528-528: New spy function added for value changesThe
valueChangeSpywill be useful for tracking and asserting date value changes in the new tests. This is a good testing practice for verifying component behavior.
Line range hint
1-646: Comprehensive test coverage for DatePicker componentThe additions to this test file significantly improve the coverage of the DatePicker component, particularly for the month and year picker functionality. The new tests are well-integrated with the existing test suite and follow established patterns. This enhancement will help ensure the reliability and correctness of the DatePicker component.
good.calendar.mp4
|
Accordion no longer changed.
Narrowed scope.
Is that correct now? |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
wingkwong
requested review from
ryo-manba
and removed request for
jrgarciadev
wingkwong
changed the title
| @@ -99,6 +101,7 @@ export function CalendarHeader(props: CalendarHeaderProps) { | |||
| return showMonthAndYearPickers ? ( | |||
| <Button | |||
| {...headerProps} | |||
| aria-label={monthAndYearPickerToggle} | |||
There was a problem hiding this comment.
we don't hardcode aria-label internally. We can take it from users' side tho.
|
Please suggest an alternative way to mark this component so the test can
find it.
…On Wed, 25 Sept 2024, 14:04 աӄա, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In packages/components/calendar/src/calendar-header.tsx
<#3789 (comment)>:
> @@ -99,6 +101,7 @@ export function CalendarHeader(props: CalendarHeaderProps) {
return showMonthAndYearPickers ? (
<Button
{...headerProps}
+ aria-label={monthAndYearPickerToggle}
we don't hardcode aria-label internally. We can take it from users' side
tho.
—
Reply to this email directly, view it on GitHub
<#3789 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBL2M6NRIQAJMCWW6L6TKF3ZYKYEDAVCNFSM6AAAAABOU7COTKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMRYGEYDCOBRGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
packages/components/calendar/src/calendar-picker-item.tsx (1)
15-15: LGTM! Consider updating prop types for consistency.The addition of the
onKeyUpprop enhances the component's event handling capabilities, which is a positive change. This aligns well with React's event handling patterns and improves the component's flexibility.For consistency, consider updating the
AriaButtonPropstype to include theonKeyUpprop if it's not already included. This ensures type safety and better documentation of the component's API.packages/components/calendar/src/calendar-picker.tsx (2)
93-93: LGTM: onKeyUp handler added to month pickerThe addition of the
onKeyUpevent handler to the monthCalendarPickerItemis consistent with the new functionality. This will improve keyboard interaction for month selection.For consistency with other event handlers, consider using an arrow function:
- onKeyUp={(e) => onPickerItemKeyUp(e, month.value, "months")} + onKeyUp={e => onPickerItemKeyUp(e, month.value, "months")}
115-115: LGTM: onKeyUp handler added to year pickerThe addition of the
onKeyUpevent handler to the yearCalendarPickerItemis consistent with the new functionality and mirrors the change made to the month picker. This will improve keyboard interaction for year selection.For consistency with other event handlers, consider using an arrow function:
- onKeyUp={(e) => onPickerItemKeyUp(e, year.value, "years")} + onKeyUp={e => onPickerItemKeyUp(e, year.value, "years")}packages/components/calendar/src/use-calendar-picker.ts (3)
128-142: Enhanced keyboard navigation control.The addition of preventDefault for specific key events is a good improvement. It ensures better control over keyboard navigation within the picker.
Consider extracting the array of keys into a constant for better maintainability:
const PREVENTED_KEYS = [ "ArrowDown", "ArrowUp", "Home", "End", "PageUp", "PageDown", "Escape", "Enter", " ", ]; if (PREVENTED_KEYS.includes(e.key)) { e.preventDefault(); }
180-181: Simplified scrolling logic.The direct call to
scrollTosimplifies the code and aligns with the new implementation. However, consider adding a bounds check fornextValueto prevent potential issues with out-of-range values.Consider adding a safety check:
if (nextValue >= 0 && nextValue < (list === 'months' ? months.length : years.length)) { scrollTo(nextValue, list); }
187-202: Good addition to address issue #3789.The new
onPickerItemKeyUpfunction effectively ensures that the selected item remains in view after keyboard interaction, addressing the synchronization issue mentioned in the PR objectives.Consider adding a null check for
listRef.currentto prevent potential errors:if (e.currentTarget && listRef.current) { scrollIntoView(e.currentTarget, { scrollMode: "always", behavior: "smooth", boundary: listRef.current, }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/components/calendar/src/calendar-picker-item.tsx (1 hunks)
- packages/components/calendar/src/calendar-picker.tsx (3 hunks)
- packages/components/calendar/src/use-calendar-picker.ts (6 hunks)
🔇 Additional comments (7)
packages/components/calendar/src/calendar-picker-item.tsx (2)
Line range hint
1-53: Summary: Improved event handling enhances component flexibility.The changes to the
CalendarPickerItemcomponent, specifically the addition of theonKeyUpprop and its integration with theuseAriaButtonhook, are well-implemented and align with the PR objectives. These modifications enhance the component's event handling capabilities, potentially contributing to resolving the synchronization issues mentioned in the PR description.The changes are minimal, focused, and maintain the existing structure and accessibility features of the component. This update should improve the overall functionality of the date picker without introducing breaking changes.
23-23: LGTM! Verify useAriaButton hook implementation.The addition of
onKeyUpto the useAriaButton hook call is correct and consistent with the component's updated props. This ensures that theonKeyUpevent is properly handled by the aria button implementation.To ensure full compatibility, please verify that the useAriaButton hook in the @nextui-org/use-aria-button package correctly handles the onKeyUp prop. Run the following script to check the hook's implementation:
packages/components/calendar/src/calendar-picker.tsx (2)
30-30: LGTM: New event handler addedThe addition of
onPickerItemKeyUpto the destructured properties fromuseCalendarPickerhook is consistent with the intended changes. This new handler will enhance the keyboard interaction capabilities of the component.
30-30: Overall impact: Improved keyboard interaction for date pickerThe addition of
onKeyUpevent handlers for both month and year picker items aligns well with the PR objectives. These changes should improve the synchronization between the selected date and the UI, addressing the issue described in #3785.To ensure the changes fully resolve the reported bug:
Also applies to: 93-93, 115-115
packages/components/calendar/src/use-calendar-picker.ts (3)
6-6: LGTM: Import statement updated correctly.The addition of
useEffectto the import statement is appropriate, given the new side-effect management introduced in the component.
100-102: Improved date focusing logic.The direct update of the focused date in the
scrollTofunction is a good improvement. It simplifies the logic and reduces the potential for synchronization issues that could occur with the previous scroll position calculations.This change aligns well with the PR objective of resolving synchronization issues in the date picker UI.
Line range hint
1-221: Overall assessment: Improvements align well with PR objectives.The changes in this file effectively address the synchronization issue in the date picker UI as outlined in the PR objectives. The removal of complex scroll handling logic and the introduction of more direct state updates simplify the code and reduce the potential for synchronization errors. The new keyboard navigation controls and the addition of the
onPickerItemKeyUpfunction further enhance the component's usability and accessibility.A few minor suggestions have been made for additional safety checks and code organization, but these are not critical. The overall changes represent a significant improvement in the component's functionality and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/components/calendar/src/use-calendar-picker.ts (3)
128-142: Enhanced keyboard navigation in onPickerItemKeyDownThe addition of a check to prevent default behavior for specific keys is a valuable improvement to the
onPickerItemKeyDownfunction. This change enhances keyboard navigation and accessibility, aligning with the PR objective of improving the date picker's functionality.However, to improve readability, consider extracting the array of keys into a constant:
const PREVENTED_KEYS = [ "ArrowDown", "ArrowUp", "Home", "End", "PageUp", "PageDown", "Escape", "Enter", " ", ]; if (PREVENTED_KEYS.includes(e.key)) { e.preventDefault(); }This refactoring would make the code more maintainable and easier to update in the future.
187-202: New onPickerItemKeyUp function improves UI consistencyThe addition of the
onPickerItemKeyUpfunction is a valuable improvement:
- It addresses the issue mentioned in fix(date-picker): month and year picker scroller #3789 by ensuring the selected item is always visible after a key event.
- The use of smooth scrolling enhances the user experience.
- The function is properly memoized with useCallback for performance optimization.
To further improve the code, consider extracting the
scrollIntoViewoptions into a constant, as they are used in multiple places:const SCROLL_OPTIONS = { scrollMode: "always" as const, behavior: "smooth" as const, }; // Usage scrollIntoView(e.currentTarget, { ...SCROLL_OPTIONS, boundary: listRef.current, });This would reduce duplication and make it easier to maintain consistent scrolling behavior across the component.
Line range hint
1-222: Overall assessment: Significant improvements to date picker functionalityThis PR successfully addresses the main objectives:
- Fixes the synchronization issue between the month and year in the date picker UI.
- Improves keyboard navigation and accessibility.
- Enhances the overall user experience of the date picker component.
The changes are well-implemented and follow good coding practices. The removal of the
handleListScrollfunction and the simplification of thescrollTofunction have streamlined the code, making it more maintainable.To further improve the code:
- Consider extracting repeated code patterns into constants or utility functions, as suggested in previous comments.
- Add comments explaining the purpose of the new
onPickerItemKeyUpfunction and how it relates to issue fix(date-picker): month and year picker scroller #3789.- Update the component's documentation to reflect these new behaviors and keyboard interactions.
Great work on addressing the reported issues and enhancing the component's functionality!
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/components/calendar/src/use-calendar-picker.ts (7 hunks)
🔇 Additional comments (3)
packages/components/calendar/src/use-calendar-picker.ts (3)
6-6: LGTM: Import statement updated to include useEffectThe addition of
useEffectto the import statement is appropriate given the changes in the component's lifecycle management. This change aligns with the removal of thehandleListScrollfunction and indicates a shift towards using React's built-in hooks for managing side effects.
100-102: Improved date synchronization in scrollTo functionThe simplification of the
scrollTofunction, particularly the direct update of the focused date usingstate.setFocusedDate, is a significant improvement. This change:
- Enhances readability and maintainability of the code.
- Ensures immediate synchronization between the UI and the state.
- Directly addresses the synchronization issue mentioned in the PR objectives.
This modification effectively resolves the core problem described in issue #3785, where the displayed year did not match the selected year.
158-161: Improved 'Home' and 'End' key functionalityThe updates to the 'Home' and 'End' key handling in the
onPickerItemKeyDownfunction are well-implemented:
- Setting
nextValueto 1 for the 'Home' key ensures navigation to the start of the list.- Using
months.lengthfor the 'End' key is a more dynamic and correct approach, ensuring navigation to the end of the list regardless of the number of months.These changes enhance the user experience by providing more intuitive and reliable navigation within the date picker.
|
I had one more idea, which I've added. The PickerItems now also have a keyUp listener, which just calls the scrollIntoView function. This seems to be a bullet proof way of ensuring that if you've been holding down any of the directional keys, then whatever was the last key to fire (and thus has focus) is scrolled to the desired position. I have to confess my weaker experience with writing imperative DOM events, so please if anyone with that could ponder whether this broad-brush fix could have any unexpected side-effects? I also changed the home/end values of the keyDown event to 1 and 12, since months are 1-indexed, not zero indexed. These buttons don't work for the year picker, since 1 and 12 aren't valid years within the available range. Couldn't see an obvious way to pass in the max/min years that would be dynamically set (since it could vary with locale etc.). Hopefully this wraps up the functionality of this component & hook? |
| expect(valueChangeSpy).not.toHaveBeenCalled(); | ||
| expect(monthBefore).toHaveAttribute("tabindex", "0"); | ||
| expect(yearBefore).toHaveAttribute("tabindex", "0"); |
There was a problem hiding this comment.
Please also check the following two points:
- Ensure that the value in the header is synchronized.
- Verify that pressing the Enter key triggers the
valueChangeSpy.
|
The Home/End keys seem to already function in the original implementation. Where did you find the information about the month's index?
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
packages/components/calendar/src/use-key-repeat-blocker.ts (2)
10-51: LGTM: Well-implemented custom hook with proper use of React hooks.The
useKeyRepeatBlockerhook is well-implemented, making appropriate use ofuseReffor mutable state anduseCallbackfor function memoization. The logic for blocking key repeats is sound and aligns with React best practices.A minor suggestion for improvement:
Consider adding a cleanup function using
useEffectto reset thekeyPressedRefwhen the component unmounts. This can prevent potential memory leaks in long-running applications.useEffect(() => { return () => { keyPressedRef.current = {}; }; }, []);
1-51: Excellent implementation addressing PR objectives.This new
useKeyRepeatBlockerhook is a well-crafted solution that directly addresses the PR's objective of improving date picker functionality. By providing a reusable mechanism for blocking key repeats, it can effectively resolve the synchronization issues mentioned in the PR summary.Key points:
- The hook is implemented efficiently, using
useRefanduseCallbackto minimize re-renders.- It's type-safe and well-documented through clear interface definitions.
- The solution is generic enough to be potentially useful beyond just the date picker, promoting code reuse.
This implementation should significantly contribute to resolving the UI rectangle intersection error mentioned in the PR objectives, as it provides fine-grained control over key press handling.
Consider documenting this hook in your project's shared components or utilities, as it could be beneficial for other UI components that require similar key repeat blocking functionality.
packages/components/calendar/src/use-calendar-picker.ts (4)
6-6: LGTM! Consider using an enum for LISTENED_NAVIGATION_KEYS.The new imports and constants are well-defined and support the updated scrolling and key navigation behavior. Good job on extracting these values for better maintainability.
Consider using an enum for
LISTENED_NAVIGATION_KEYSto improve type safety:enum NavigationKeys { ArrowDown = "ArrowDown", ArrowUp = "ArrowUp", Home = "Home", End = "End", PageUp = "PageUp", PageDown = "PageDown", Escape = "Escape", Enter = "Enter", Space = " ", } const LISTENED_NAVIGATION_KEYS = Object.values(NavigationKeys);Also applies to: 11-12, 27-50
151-167: Great improvements to scroll and key handling!The introduction of
useScrollEndCallbackanduseKeyRepeatBlockerhooks, along with thegetBoundaryValuefunction, significantly improves the handling of scroll events, key repeat behavior, and boundary values. The update to thescrollTofunction to set the focused date is a good synchronization improvement.Consider adding a comment explaining the purpose of
OF_100_MILLISECONDSto improve code readability:// Delay for scroll end detection in milliseconds const OF_100_MILLISECONDS = 100;Also applies to: 127-129
235-262: Excellent addition to fix synchronization issues!The new
onPickerItemKeyUpfunction is a great solution to address the synchronization issue mentioned in issue #3789. The safety scroll implementation ensures that the UI stays in sync with keyboard navigation, preventing the UI rectangle intersection error.Consider adding a brief comment explaining the purpose of this function and how it addresses the synchronization issue:
// Ensures UI synchronization after key navigation, addressing issue #3789 const onPickerItemKeyUp = useCallback( // ... (rest of the function) );
277-277: Great overall improvements to the date picker functionality!The changes in this PR successfully address the synchronization issues between the month and year in the date picker UI. The new key handling logic, improved scrolling behavior, and the addition of
onPickerItemKeyUpcollectively enhance the user experience and resolve the reported bugs.Consider adding unit tests for the new
onPickerItemKeyUpfunction and the updatedonPickerItemKeyDownto ensure the correct behavior is maintained in future updates. This will help prevent regression and improve the overall robustness of the component.packages/components/calendar/src/use-scroll-end-callback.ts (1)
50-50: Remove redundant commentThe comment "You can adjust the delay as necessary" is unnecessary because the
debounceduration is already adjustable through the parameter. Removing it cleans up the code.Apply this diff:
}, debounce); // You can adjust the delay as necessary + }, debounce);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- packages/components/calendar/src/use-calendar-picker.ts (7 hunks)
- packages/components/calendar/src/use-key-repeat-blocker.ts (1 hunks)
- packages/components/calendar/src/use-scroll-end-callback.ts (1 hunks)
🔇 Additional comments (3)
packages/components/calendar/src/use-key-repeat-blocker.ts (2)
1-1: LGTM: Imports are appropriate and minimal.The imports of
useCallbackanduseReffrom 'react' are suitable for this implementation, providing the necessary hooks without any superfluous imports.
3-8: LGTM: Well-defined interface for the hook's return value.The
UseKeyRepeatBlockerReturninterface is clearly defined, providing a robust type structure for the hook's return value. The method signatures are descriptive and use TypeScript effectively for type safety.packages/components/calendar/src/use-calendar-picker.ts (1)
Line range hint
169-233: Excellent improvements to key handling and navigation!The updates to
onPickerItemKeyDownsignificantly enhance the key navigation experience. The new logic for Home and End keys usinggetBoundaryValueimproves boundary handling. The improved scrolling and focus management should resolve synchronization issues.This change addresses the previous comment by ryo-manba about the Home/End key behavior:
- The year is now changing correctly.
- The smooth scrolling behavior has been maintained while ensuring correct focus management.
Great job on implementing these improvements!
| const abortRef = useRef<(() => void) | null>(); | ||
| const onScrollRef = useRef<(() => void) | null>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize abortRef and onScrollRef to null
Currently, abortRef and onScrollRef are declared without initial values. Initializing them explicitly to null enhances code clarity and ensures consistent types.
Apply this diff:
- const abortRef = useRef<(() => void) | null>();
- const onScrollRef = useRef<(() => void) | null>();
+ const abortRef = useRef<(() => void) | null>(null);
+ const onScrollRef = useRef<(() => void) | null>(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const abortRef = useRef<(() => void) | null>(); | |
| const onScrollRef = useRef<(() => void) | null>(); | |
| const abortRef = useRef<(() => void) | null>(null); | |
| const onScrollRef = useRef<(() => void) | null>(null); |
| const clearListener = useCallback(() => { | ||
| if (elementRef.current && onScrollRef.current) { | ||
| elementRef.current.removeEventListener("scroll", onScrollRef.current); | ||
| } | ||
| // Remove the event listener and clear timeout when the component unmounts | ||
| if (abortRef.current) { | ||
| abortRef.current(); | ||
| } | ||
| abortRef.current = null; | ||
| onScrollRef.current = null; | ||
| elementRef.current = null; | ||
| }, [elementRef]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust the dependency array of clearListener
The clearListener function includes elementRef in its dependency array. Since elementRef is a ref object that doesn't change identity, and you are accessing elementRef.current, it's safe to use an empty dependency array. This prevents unnecessary re-creations of the callback.
Apply this diff:
- }, [elementRef]);
+ }, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const clearListener = useCallback(() => { | |
| if (elementRef.current && onScrollRef.current) { | |
| elementRef.current.removeEventListener("scroll", onScrollRef.current); | |
| } | |
| // Remove the event listener and clear timeout when the component unmounts | |
| if (abortRef.current) { | |
| abortRef.current(); | |
| } | |
| abortRef.current = null; | |
| onScrollRef.current = null; | |
| elementRef.current = null; | |
| }, [elementRef]); | |
| const clearListener = useCallback(() => { | |
| if (elementRef.current && onScrollRef.current) { | |
| elementRef.current.removeEventListener("scroll", onScrollRef.current); | |
| } | |
| // Remove the event listener and clear timeout when the component unmounts | |
| if (abortRef.current) { | |
| abortRef.current(); | |
| } | |
| abortRef.current = null; | |
| onScrollRef.current = null; | |
| elementRef.current = null; | |
| }, []); |
| (element: HTMLElement | null, callback: () => void) => { | ||
| if (!element) return; | ||
| elementRef.current = element; | ||
|
|
There was a problem hiding this comment.
Prevent multiple listeners by clearing previous ones
When onScrollEnd is called multiple times with different elements, the previous scroll event listeners remain attached, leading to potential memory leaks or unexpected behavior. Before setting up a new listener, ensure that any existing listener is removed.
Apply this diff to fix the issue:
const onScrollEnd = useCallback(
(element: HTMLElement | null, callback: () => void) => {
+ clearListener();
if (!element) return;Committable suggestion was skipped due to low confidence.
| [debounce], | ||
| ); | ||
|
|
||
| return {onScrollEnd, abortRef}; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider returning abortRef
Currently, the hook returns abortRef, which exposes an internal ref object. For a cleaner API, consider returning the abort function directly or providing an abort method. This makes it more intuitive for consumers to cancel the timeout or listener without accessing internal refs.
Example modification:
return { onScrollEnd, abortRef };
+ // Instead of returning abortRef, consider returning the abort function
+ return { onScrollEnd, abort };Committable suggestion was skipped due to low confidence.
|
I added two additional hooks to cover the different behaviours needed for the different navigation keys:
If you have existing hooks with the required API already in the codebase, feel free to swap them in. Equally, feel free to migrate them to a different (generic) hook folder if you want to centralize them for other scenarios to use. I can't find any more bugs in this component. On a side note, I prefer the simpler version where home/end cause the instant scrolling. I get that this is a minor inconsistency in the style of the UI, but the two extra control flow checks to defer the focus update and block repeat key strokes seems overly convoluted. I wouldn't be surprised to find it has negative performance implications or introduces other bugs on different systems from my own... Let me know what you think! :-) |
|
Yep there was one more bug where the listener wasn't clearing correctly. Fixed that now. cries in open source |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/components/calendar/src/use-calendar-picker.ts (2)
151-167: Enhanced key event handling and boundary value managementThe additions of
abortRef,handleKeyDown,handleKeyUp, andisKeyDownimprove the handling of key events. ThegetBoundaryValuefunction provides a reusable way to determine min/max values for months and years, enhancing code maintainability.Consider adding type annotations to the
getBoundaryValuefunction for improved readability:const getBoundaryValue = useCallback( (list: CalendarPickerListType, bound: "min" | "max"): number => { // ... existing implementation }, [minValue, maxValue] );
235-258: Enhanced key up event handlingThe new
onPickerItemKeyUpfunction improves the reliability of the picker's navigation:
- It handles key up events for navigation keys, preventing default behavior when necessary.
- Implements a safety scroll to the element that fired the event, addressing issue fix(date-picker): month and year picker scroller #3789.
- Uses different behavior for keys that need deferred focus, improving user experience.
Consider adding error handling for the
scrollIntoViewfunction to gracefully handle any potential errors:try { scrollIntoView(e.currentTarget, { scrollMode: "always", behavior: "smooth", boundary: listRef.current, }); } catch (error) { console.error("Error scrolling into view:", error); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/components/calendar/src/use-calendar-picker.ts (7 hunks)
- packages/components/calendar/src/use-scroll-end-callback.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/calendar/src/use-scroll-end-callback.ts
🔇 Additional comments (4)
packages/components/calendar/src/use-calendar-picker.ts (4)
6-6: Improved event handling and code organizationThe new imports, custom hooks, and constants enhance the component's functionality and improve code readability:
- The addition of
useCallback,useEffect, anduseRefsuggests optimized rendering and better management of side effects.- Custom hooks
useScrollEndCallbackanduseKeyRepeatBlockerindicate improved handling of scroll and key events.- New constants provide clear definitions for boundary values and key handling, making the code more maintainable.
These changes contribute to a more robust and efficient implementation of the calendar picker.
Also applies to: 11-12, 27-50
273-273: Extended hook return valueThe addition of
onPickerItemKeyUpto the returned object fromuseCalendarPickeris appropriate. This change makes the new key up event handling function available to the component using this hook, allowing for improved interaction handling.
Line range hint
1-278: Overall implementation reviewThe changes in this file significantly improve the calendar picker's functionality and user experience:
- Enhanced key navigation and scroll behavior, addressing the issues with Home/End keys.
- Improved event handling with the introduction of custom hooks and new utility functions.
- Better code organization and readability through the use of constants and clear function definitions.
- Addressed synchronization issues between the month and year in the date picker UI, as mentioned in the PR objectives.
These improvements align well with the PR objectives and resolve the issues mentioned in the past review comments. The code is well-structured, follows good practices, and should provide a more reliable and user-friendly date picker component.
Line range hint
168-233: Improved key navigation and scroll behaviorThe changes in the
onPickerItemKeyDownfunction significantly improve the calendar picker's navigation:
- The new logic for Home and End keys now correctly updates the year and provides smooth scrolling.
- The implementation of deferred focus for Home and End keys enhances the user experience.
- The use of
abortRefandonScrollEndensures proper management of scroll behavior.These improvements address the issues mentioned in the past review comments about Home/End key behavior.
To ensure that the Home/End key behavior is fully fixed, please run the following test script:
✅ Verification successful
Update Verification Scripts for Home/End Key Behavior
The previous scripts failed to recognize the
typescriptfile type. Updated scripts now target.tsfiles directly.
Home/End Key Behavior Verified
The implementation of Home/End key handling, year updates, and smooth scrolling in the
onPickerItemKeyDownfunction has been successfully verified based on the shell script results.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Home/End key behavior in the calendar picker # Test: Search for the implementation of Home and End key handling rg --type typescript 'case "Home":|case "End":' packages/components/calendar/src/use-calendar-picker.ts # Test: Check if the year is being updated rg --type typescript 'nextValue = getBoundaryValue\(list, "(min|max)"\);' packages/components/calendar/src/use-calendar-picker.ts # Test: Verify smooth scrolling implementation rg --type typescript 'scrollTo\(nextValue, list\);' packages/components/calendar/src/use-calendar-picker.tsLength of output: 452
Script:
#!/bin/bash # Description: Verify the Home/End key behavior in the calendar picker # Test: Search for the implementation of Home and End key handling rg 'case "Home":|case "End":' --glob '*.ts' packages/components/calendar/src/use-calendar-picker.ts # Test: Check if the year is being updated rg 'nextValue = getBoundaryValue\(list, "(min|max)"\);' --glob '*.ts' packages/components/calendar/src/use-calendar-picker.ts # Test: Verify smooth scrolling implementation rg 'scrollTo\(nextValue, list\);' --glob '*.ts' packages/components/calendar/src/use-calendar-picker.tsLength of output: 554
This seems to be a separate issue from the current one. If you'd like to address it, please open a new issue for further discussion. |
| @@ -0,0 +1,69 @@ | |||
| import {useCallback, useEffect, useRef} from "react"; | |||
|
|
|||
| function useScrollEndCallback(debounce: number) { | |||
There was a problem hiding this comment.
The value is not being synced during mouse scroll. Please fix it.
There was a problem hiding this comment.
Could you please specify the tests you need? Given that original component had tests that failed to catch this bug, it's a somewhat open-ended task. My current codebase uses with a different UI library, so it's difficult to justify spending any more time on the NextUI repo at the moment, but if you can ring-fence exactly what needs doing I will make time for that.
| expect(valueChangeSpy).not.toHaveBeenCalled(); | ||
| expect(monthBefore).toHaveAttribute("tabindex", "0"); | ||
| expect(yearBefore).toHaveAttribute("tabindex", "0"); |
|
Any updates on this PR? |
Closes #3785
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
No.
📝 Additional Information
Summary by CodeRabbit
Summary by CodeRabbit
AccordionTreecomponent for hierarchical data representation.@nextui-org/accordionand@nextui-org/calendarpackages.DatePickercomponent.