-
-
Notifications
You must be signed in to change notification settings - Fork 476
Feat/date value datepicker #1190
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 value datepicker #1190
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1190 +/- ##
==========================================
- Coverage 99.54% 97.36% -2.19%
==========================================
Files 163 214 +51
Lines 6621 9091 +2470
Branches 401 542 +141
==========================================
+ Hits 6591 8851 +2260
- Misses 30 240 +210 ☔ View full report in Codecov by Sentry. |
|
Hey @rluders , I guess now it is ready for review, could please czech it out ? |
|
@ddiasfront I see some formatting issues |
|
@SutuSebastian Can you describe it, please? I didn't see any formatting issues regarding the date itself, is it on the date or another stuff? |
Apparently u fixed it with the last commits :D The build failed, and inside the build fail output there was the formatting issue AFAIR. |
|
@rluders , @SutuSebastian Okay guys, now I've fixed the issues bellow as well, including the clear functionality: In the clear functionality scenario, I had to add a label property to be rendered whenever there's no date selected, and i set it to default "No date selected", please check it out. |
02d1f8f to
4548a0c
Compare
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: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- apps/web/content/docs/components/datepicker.mdx (1 hunks)
- packages/ui/src/components/Datepicker/Datepicker.spec.tsx (7 hunks)
- packages/ui/src/components/Datepicker/Datepicker.stories.tsx (4 hunks)
- packages/ui/src/components/Datepicker/Datepicker.tsx (7 hunks)
- packages/ui/src/components/Datepicker/DatepickerContext.tsx (1 hunks)
- packages/ui/src/components/Datepicker/Views/Days.tsx (1 hunks)
- packages/ui/src/components/Datepicker/Views/Decades.tsx (2 hunks)
- packages/ui/src/components/Datepicker/Views/Months.tsx (2 hunks)
- packages/ui/src/components/Datepicker/Views/Years.tsx (1 hunks)
Additional comments not posted (33)
packages/ui/src/components/Datepicker/DatepickerContext.tsx (1)
17-18: The past review comment on lines 17-17 is still valid and applicable to the current changes. It has already addressed the need to updatesetSelectedDateto handlenullvalues for consistency with theselectedDateproperty.packages/ui/src/components/Datepicker/Views/Years.tsx (1)
37-37: LGTM!The conditional check for the truthiness of
selectedDatebefore callingisDateEqualis a good defensive programming practice. It enhances the robustness of the date selection logic by preventing unnecessary function calls and potential runtime errors whenselectedDateis not defined.packages/ui/src/components/Datepicker/Views/Months.tsx (2)
5-5: LGTM!The import statement has been correctly updated to include the
isDateEqualfunction from the../helpersmodule. This change is necessary to support the updated logic in theisSelectedconstant.
45-45: Approve the updated selection logic.The changes to the
isSelectedconstant are an improvement over the previous implementation:
Using
isDateEqualinstead ofisMonthEqualensures that the entire date object is compared for equality, not just the month. This provides a more accurate determination of whether a month is selected.Checking if
selectedDateis defined before performing the equality check enhances the robustness of the selection logic by avoiding potential errors whenselectedDateis undefined.These changes positively impact the user experience by ensuring that the selected month is accurately highlighted in the date picker.
packages/ui/src/components/Datepicker/Views/Decades.tsx (2)
36-36: LGTM!The conditional check for
selectedDatebefore callingisDateInDecadeis a good improvement. It prevents potential errors whenselectedDateis null or undefined.
52-52: LGTM!The change to adjust the
viewDatebased on the difference between theyearand the year of theselectedDateis a good improvement. It allows for a more dynamic adjustment of the view date, reflecting the selected decade more accurately. The conditional check forselectedDatealso prevents potential errors.packages/ui/src/components/Datepicker/Views/Days.tsx (1)
58-58: LGTM!The updated logic for determining the selected date is more robust and handles the case when
selectedDateis undefined. This change aligns with the PR objective of supporting controlled inputs and prevents potential errors.apps/web/content/docs/components/datepicker.mdx (1)
72-76: LGTM!The new section provides a clear explanation of how to create a controlled datepicker using the
valueprop. The example enhances the documentation by demonstrating the functionality in practice.The changes align with the PR objective of introducing the
dateValueprop for controlled usage and are consistent with the AI summary.packages/ui/src/components/Datepicker/Datepicker.stories.tsx (5)
35-63: LGTM!The
ControlledTemplatestory is a great addition to demonstrate the controlled behavior of the Datepicker component. The usage of React'suseStateanduseEffecthooks to manage the selected date based on thevalueprop is implemented correctly. The story also handles the conversion ofminDateandmaxDateprops to Date objects and modifies thedefaultValuebased on the provided date range, ensuring that the default date falls within the specified limits.
74-79: LGTM!The
Templatestory is correctly updated to align with the new logic. The handling ofdefaultDateis changed todefaultValueand it adheres to the same date range constraints.
84-96: LGTM!The
ControlledDefaultEmptystory is a great addition to demonstrate the controlled behavior of the Datepicker component with no selected date. Setting thevalueprop tonulland thelabelprop to "No date selected" is a good way to showcase this scenario.
111-121: LGTM!The
NullDateValuestory is a good addition to demonstrate the behavior of the Datepicker component with a null date value.
123-134: LGTM!The
DateValueSetstory is a good addition to demonstrate the behavior of the Datepicker component with a default date value. Setting thedefaultValueprop to the current date usingnew Date()is a nice way to showcase this scenario.packages/ui/src/components/Datepicker/Datepicker.spec.tsx (8)
60-70: LGTM!The test case has been correctly updated to use the new
onChangeprop, which replaces the previousonSelectedDateChangedprop. The test case logic remains the same and is correct.
83-83: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
99-99: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
116-116: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
133-133: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
150-150: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
170-170: LGTM!The test case has been correctly updated to use the new
defaultValueprop, which replaces the previousdefaultDateprop. The test case logic remains the same and is correct.
Line range hint
1-220: LGTM!The test cases in the
Datepicker.spec.tsxfile have been correctly updated to use the new prop names:
onSelectedDateChangedhas been renamed toonChange.defaultDatehas been renamed todefaultValue.The test case logic remains the same and is correct. The changes are consistent with the list of alterations provided.
packages/ui/src/components/Datepicker/Datepicker.tsx (12)
4-4: LGTM!The additional imports are necessary for the changes made in the file.
80-80: LGTM!The comment update accurately reflects the changes made to the
clearmethod implementation.
85-101: LGTM!The changes to the
DatepickerPropsinterface are in line with the PR objectives and the AI-generated summary. The new props enhance the component's flexibility and usability by allowing for better integration with external state management.
114-123: LGTM!The destructuring of the new props is necessary for using them in the component implementation.
138-140: LGTM!The initialization logic for
selectedDateandviewDatestate variables is correct and handles the controlled and uncontrolled scenarios properly.
Line range hint
146-162: LGTM!The changes to the
changeSelectedDateandclearDatefunctions are in line with the PR objectives and the AI-generated summary. The modifications ensure that the selected date is updated correctly and theonChangecallback is invoked with the updated date.
250-259: LGTM!The new
useEffecthook is necessary to keep theselectedDatestate in sync with the controlledvalueprop. The logic correctly handles the controlled scenario and ensures that the selected date is within the allowed range.
261-262: LGTM!The introduction of the
displayValuevariable is a good approach to handle the display logic for the input field. It ensures that the input reflects the current state accurately and falls back to thelabelprop when no date is selected.
289-292: LGTM!Updating the
viewDatestate toselectedDatewhen the input field is focused ensures that the calendar view is synchronized with the selected date, providing a better user experience.
294-294: LGTM!Setting the
valueprop of theTextInputcomponent todisplayValueensures that the input field displays the formatted selected date or the label when no date is selected.
296-296: LGTM!Setting the
defaultValueprop of theTextInputcomponent to the formattedinitialDateor thelabelprop ensures that the input field displays the correct initial value when the component is uncontrolled.
359-359: LGTM!Passing
nullas the date value to thechangeSelectedDatefunction in the clear button'sonClickhandler ensures that the selected date is properly cleared when the clear button is clicked.
Alright, done guys, changeset added, shoutout anything else might need. |
all good 🔥 |
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.
we have to re-do the changeset, it is not a major, u can leave it as patch
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.
@SutuSebastian, @rluders updated changeset
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.
nice, can u also put some more info in regards of the breaking changes and what users have to modify when they update to this next version of the datepicker?
U can do this in the same changeset markdown file
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
🧹 Outside diff range and nitpick comments (1)
.changeset/old-jobs-decide.md (1)
5-13: Consider adding information about clear functionality and label property.The Datepicker Component Updates section provides a good overview of the improvements. However, it would be beneficial to explicitly mention:
- The addition of clear functionality (related to Issue #1039).
- The new label property that displays "No date selected" when no date is chosen.
These additions would provide a more comprehensive summary of the changes made.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- .changeset/old-jobs-decide.md (1 hunks)
🔇 Additional comments (2)
.changeset/old-jobs-decide.md (2)
15-22: Verify completeness of updated test files.The Files Updated section provides a good overview of the main files changed. However, to ensure comprehensive test coverage for all the new features and changes:
- Verify if any additional test files were updated or created, especially for the new controlled input functionality and clear feature.
- Consider mentioning any updates to integration or end-to-end tests, if applicable.
This will help ensure that all aspects of the changes are properly tested and documented.
#!/bin/bash # Description: Check for any new or modified test files related to Datepicker # Test: Search for new or modified test files. Expect: List of test files if any were changed or added. git diff --name-only origin/main | grep -E 'test|spec'
1-3:⚠️ Potential issueVerify the changeset type for breaking changes.
The changeset specifies a patch update, which is typically used for non-breaking changes and bug fixes. However, the PR description mentions a breaking change (renaming of
onSelectedDateChangetoonChange). Consider updating the changeset to a minor or major version bump if this breaking change is intended to be included.
@tulup-conner can we close this thread and head towards merging ? |
Yes, please. Let's get it merged. It is already too much to handle in here. I didn't notice anything that could cause problems. |
SutuSebastian
left a comment
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.
awesome
|
Heads up that this ended up being a breaking change for typescript users due to changed types. Might have been worth a minor (=major due to 0.) bump. I'll add details later. Errors from our CI: |
* feat(datepicker): accepts dateValue as value This issue was found in a scenario where I needed to update the selectedDate without clicking into the component, just by passing a value to the Datepicker, and actually it just accepts changes by clicking which I don't believe covers the use cases. * chore(lint/prettier): cleaning code Cleaning code * removed unecessary debug * test(fixing tests): finding why tests are not running Tests stopped running locally, running it on the pipe * chore: fixed testing for the use case scenario * fix: removing commments Comments removal * chore(reducing complexity for datevalue): assertive state/context management according view/date * chore: linting * feat: added necessary documentation * Update Datepicker.spec.tsx chore: removing screen.debug * chore: pipeline build fixes * fix(clear button): fixed datepicker clear button to not displaying any selected date by using null" Attributed null as a acceptable value to the selectedDate, also removed default args for weekstart which was crashing the app due to an overlap with the component props 1039 * Update Datepicker.spec.tsx * test(datepicker): clear Clear should affect dateValue * docs(datepicker): it should have right type for the new empty date case scenario texting property Updated datepicker component storybook file controls, added the new property and its type so the dev can simulate scenarios on storybook * chore: removing unecessary console * docs(datepicker): label naming Changed the name from labelempty to label * feat(datepicker): datevalue expects null to clear date Now datevalue expects null to clear date, also a diff mechanism was added on the dateValue useffect to prevent mass re-renders. g * feat(datepicker): datepicker should expect value and defaultValue Now datepicker component accepts value and defaultValue as props comes from this is the most intuitive pattern. BREAKING CHANGE: A breaking change which affects the old property defaultDate being named now defaultValue. * feat(datepicker): datepicker default value and sideeffects Setting defaultvalue to empty when the component loads, therefore updating its sideffects on mounting considering dateview initialization | Added test cases to these scenarios | Added propper naming and default values according flowbite patterns BREAKING CHANGE: New defaultValue initialization value | Migrating from defaultDate to defaultValue * feat: adding datevalue property * feat: handling date value with propper test cases * feat: handling date value with propper test cases * feat: removed default value and renamed onchange prop * feat: solved comments | updated tests and parameters * feat: ts compiling * feat: added null as a type reference for selectedDate * feat: prettier * feat: added value as a modifier * feat: updated storybook to reflect controlled and uncontrolled model * feat: removed debugger and renamed defaultDate to defaultValue * feat: removed debugger and renamed defaultDate to defaultValue * feat: format check * feat: addressing hook dependencies * feat: controlled component setup | clearn functionality * feat: remove debugger statement * feat: defaultDate empty and side-effects addressed * feat: added changeset * feat - changset to patch --------- Co-authored-by: Dias, Diego <diasd@ae.com>
It was added a new property called dateValue, which directly modifies the date inside of the datepicker whenever it's necessary, also the view is set to this selected date, automatically, independently from the type of view.
It closes #1187 #1055 #1039 #1167
The prop added is merely optional and will not cause any breaking change, test coverage is basically set and documentation is covered as well.
The PR introduces a breaking change for the Datepicker component.
Now the 'onSelectedDateChange' property changed name to 'onChange' as it is a more approachable/obvious pattern to follow.
We don't have any other occurrences for 'onSelectedDateChange' on the codebase ATM, so it is SAFE bro, typescript will do the rest of the work :)
Summary by CodeRabbit
Summary by CodeRabbit
New Features
value,defaultValue, andlabelcontrols for enhanced Datepicker customization.valueprop.Improvements
valueanddefaultValueprops.Documentation
nullvalues and practical examples for controlled usage.Tests