-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat(date-picker): added isOpen and onOpenChange #3476
base: canary
Are you sure you want to change the base?
feat(date-picker): added isOpen and onOpenChange #3476
Conversation
ci(changesets): 📦 version packages
🦋 Changeset detectedLatest commit: 67af710 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Someone is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce the ability to manage the visibility state of the Changes
Sequence Diagram(s)N/A Assessment against linked issues
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- packages/components/date-picker/src/use-date-picker-base.ts (2 hunks)
- packages/components/date-picker/src/use-date-picker.ts (2 hunks)
Additional comments not posted (5)
packages/components/date-picker/src/use-date-picker.ts (3)
91-91
: Integration ofonOpenChange
prop.The
onOpenChange
prop is correctly destructured from theuseDatePickerBase
return object.
99-102
: EnsureonOpenChange
is called appropriately.The
onOpenChange
function is called with theisOpen
parameter, and theonClose
function is triggered whenisOpen
isfalse
. This ensures the visibility of the DatePicker popover is managed correctly.
233-234
: ExposeisOpen
andonOpenChange
in the return object.The
isOpen
state andonOpenChange
function are exposed in the return object, aligning with the PR objectives to manage the DatePicker's visibility programmatically.packages/components/date-picker/src/use-date-picker-base.ts (2)
146-146
: Integration ofonOpenChange
prop.The
onOpenChange
prop is correctly destructured from theprops
object.
301-301
: ExposeonOpenChange
in the return object.The
onOpenChange
function is exposed in the return object, aligning with the PR objectives to manage the DatePicker's visibility programmatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- missing changeset
- how does it fix the linked issue? can you update the preset example also?
@wingkwong For the changeset, should it be a minor or a patch? |
mark it patch first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- .changeset/silent-countries-drum.md (1 hunks)
- packages/components/date-picker/stories/date-picker.stories.tsx (3 hunks)
Additional comments not posted (4)
.changeset/silent-countries-drum.md (1)
1-5
: Changeset file is correctly formatted and documented.The changeset specifies a patch update for the "@nextui-org/date-picker" package and provides a clear description of what was added, linking to the relevant issue.
packages/components/date-picker/stories/date-picker.stories.tsx (3)
200-200
: State initialization for visibility control added.The addition of
isOpen
state usingReact.useState
is correctly implemented to manage the visibility of the DatePicker.
260-283
: Logic to manage DatePicker visibility on date selection.The
onPress
handlers for the buttons are correctly updating theisOpen
state to close the DatePicker after a date is selected. This aligns with the expected behavior described in the issue.
296-299
: Props for controlling DatePicker visibility added.The
isOpen
andonOpenChange
props are correctly passed to the DatePicker component, allowing external control over its visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u also update the examples in docs as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- apps/docs/content/components/date-picker/preset.ts (3 hunks)
Additional comments not posted (3)
apps/docs/content/components/date-picker/preset.ts (3)
69-92
: Review the changes to buttononPress
handlers.The
onPress
handlers for the buttons "Today", "Next week", and "Next month" now include settingisOpen
tofalse
, which is intended to close theDatePicker
after a date is selected. This change directly addresses the bug described in the issue and should resolve the problem of theDatePicker
not dismissing as expected.
9-9
: Confirm the initial state forisOpen
.The initial state for
isOpen
is set tofalse
, which is appropriate as theDatePicker
should not be visible initially. However, consider if there are scenarios where theDatePicker
should be open on component mount based on props or previous user actions.Verification successful
Confirm the initial state for
isOpen
.The initial state for
isOpen
is correctly set tofalse
, and there are no conditions or props found that suggest it should be set totrue
initially.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if there are any props or conditions that should set isOpen to true initially. # Test: Search for any conditions or props that might set isOpen initially. rg --type tsx -A 5 $'isOpen={isOpen}'Length of output: 144
Script:
#!/bin/bash # Description: Verify if there are any props or conditions that should set isOpen to true initially. # Test: Search for any conditions or props that might set isOpen initially. rg --type tsx -A 5 'isOpen=\{isOpen\}'Length of output: 68
Script:
#!/bin/bash # Description: Verify if there are any props or conditions that should set isOpen to true initially. # Test: Search for any conditions or props that might set isOpen initially. rg -A 5 'isOpen=\{isOpen\}'Length of output: 11737
105-108
: Review the integration ofisOpen
andonOpenChange
props in theDatePicker
.The
isOpen
andonOpenChange
props are correctly integrated into theDatePicker
component. This setup allows external control over the visibility of theDatePicker
, which enhances its flexibility and usability in various contexts.Additionally, ensure that
onOpenChange
is properly handling all scenarios where theDatePicker
should close or open, beyond just button presses.Verification successful
Integration of
isOpen
andonOpenChange
props in theDatePicker
is correctly handled.The
isOpen
andonOpenChange
props are properly integrated into theDatePicker
component, allowing for external control over its visibility. ThesetIsOpen
function is used comprehensively across various scenarios to manage theisOpen
state, ensuring that theDatePicker
opens and closes as expected.
onOpenChange
is used in multiple components to handle state changes effectively.setIsOpen
is defined usingReact.useState
oruseState
and correctly updates theisOpen
state.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if `onOpenChange` handles all scenarios for opening and closing the DatePicker. # Test: Search for all usages of `onOpenChange` to ensure comprehensive handling. rg --type tsx -A 5 $'onOpenChange={setIsOpen}'Length of output: 168
Script:
#!/bin/bash # Description: Verify if `onOpenChange` handles all scenarios for opening and closing the DatePicker. # Test: Search for all usages of `onOpenChange` to ensure comprehensive handling. rg --type tsx -A 5 'onOpenChange=\{setIsOpen\}'Length of output: 77
Script:
#!/bin/bash # Description: Verify if `onOpenChange` handles all scenarios for opening and closing the DatePicker. # Test: Search for all usages of `onOpenChange` to ensure comprehensive handling. rg --glob '*.tsx' -A 5 'onOpenChange=\{setIsOpen\}'Length of output: 3303
Script:
#!/bin/bash # Description: Verify the comprehensive handling of `onOpenChange` by examining the definition and usage of `setIsOpen`. # Test: Search for the definition of `setIsOpen` to understand its implementation. rg --glob '*.tsx' 'const setIsOpen' # Test: Search for all usages of `setIsOpen` to ensure comprehensive handling. rg --glob '*.tsx' 'setIsOpen('Length of output: 141
Script:
#!/bin/bash # Description: Verify the comprehensive handling of `onOpenChange` by examining the definition and usage of `setIsOpen`. # Test: Search for the definition of `setIsOpen` to understand its implementation. rg --glob '*.tsx' 'setIsOpen' # Test: Search for all usages of `setIsOpen` to ensure comprehensive handling. rg --glob '*.tsx' 'setIsOpen\('Length of output: 3728
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please include test cases
I was testing while adding the test cases and i noticed a bug. Consider the below code where you toggle
Bugs:-
Note:-
Could you tell me how you want me to proceed further? Do we need to fix this or do you want me to write the test cases only for the rendering of |
Please fix the bug as well.
I couldn't reproduce in canary branch nor production. |
Could you please check by changing the The code looks something like this
|
5b9a42d
to
67af710
Compare
Closes #3457
📝 Description
Added
isOpen
prop andonOpenChange
event, so that they could be used to control visibility of theDatePicker
popover.⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
If the PR gets accepted please use my GitHub email-id (shrinidhiupadhyaya1195@gmail.com) instead of my other email-id for the Co-authored-by: message.
Summary by CodeRabbit
isOpen
andonOpenChange
functionalities to the DatePicker component for improved control over the date picker's visibility.