Skip to content
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

Merged
merged 39 commits into from
Sep 30, 2024

Conversation

ddiasfront
Copy link
Contributor

@ddiasfront ddiasfront commented Dec 14, 2023

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

    • Added value, defaultValue, and label controls for enhanced Datepicker customization.
    • Introduced a new section in documentation for creating a controlled Datepicker using the value prop.
    • Added new story variants for different date scenarios, including controlled templates.
  • Improvements

    • Enhanced Datepicker to manage dates dynamically with value and defaultValue props.
    • Optimized internal state management using React hooks for better performance.
    • Simplified logic for clearing selected dates and ensuring robust date comparisons.
  • Documentation

    • Expanded documentation to include handling null values and practical examples for controlled usage.
  • Tests

    • Introduced comprehensive unit tests for various Datepicker functionalities, ensuring accurate behavior and state management.

Copy link

vercel bot commented Dec 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
flowbite-react ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2024 0:56am
flowbite-react-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2024 0:56am

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

Attention: Patch coverage is 80.55556% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 97.36%. Comparing base (7461173) to head (baed714).
Report is 245 commits behind head on main.

Current head baed714 differs from pull request most recent head 2070c8b

Please upload reports for the commit 2070c8b to get more accurate results.

Files Patch % Lines
src/components/Datepicker/Datepicker.tsx 90.00% 3 Missing ⚠️
src/components/Datepicker/Views/Decades.tsx 0.00% 2 Missing ⚠️
src/components/Datepicker/Views/Months.tsx 0.00% 1 Missing ⚠️
src/components/Datepicker/Views/Years.tsx 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@ddiasfront ddiasfront marked this pull request as draft December 15, 2023 00:35
@ddiasfront ddiasfront marked this pull request as ready for review December 15, 2023 01:45
@ddiasfront ddiasfront changed the title Feat/date value datepicker Feat/date value datepicker - CLOSES #1187 Dec 15, 2023
@ddiasfront
Copy link
Contributor Author

Hey @rluders , I guess now it is ready for review, could please czech it out ?

@ddiasfront ddiasfront changed the title Feat/date value datepicker - CLOSES #1187 Feat/date value datepicker Dec 17, 2023
@SutuSebastian
Copy link
Collaborator

@ddiasfront I see some formatting issues

@ddiasfront
Copy link
Contributor Author

ddiasfront commented Dec 18, 2023

@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?

@SutuSebastian
Copy link
Collaborator

SutuSebastian commented Dec 18, 2023

@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.

@ddiasfront
Copy link
Contributor Author

ddiasfront commented Dec 20, 2023

@rluders , @SutuSebastian Okay guys, now I've fixed the issues bellow as well, including the clear functionality:

#1187 #1055 #1039 #1167

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 02d1f8f and 4548a0c.

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 update setSelectedDate to handle null values for consistency with the selectedDate property.

packages/ui/src/components/Datepicker/Views/Years.tsx (1)

37-37: LGTM!

The conditional check for the truthiness of selectedDate before calling isDateEqual is a good defensive programming practice. It enhances the robustness of the date selection logic by preventing unnecessary function calls and potential runtime errors when selectedDate is not defined.

packages/ui/src/components/Datepicker/Views/Months.tsx (2)

5-5: LGTM!

The import statement has been correctly updated to include the isDateEqual function from the ../helpers module. This change is necessary to support the updated logic in the isSelected constant.


45-45: Approve the updated selection logic.

The changes to the isSelected constant are an improvement over the previous implementation:

  1. Using isDateEqual instead of isMonthEqual ensures 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.

  2. Checking if selectedDate is defined before performing the equality check enhances the robustness of the selection logic by avoiding potential errors when selectedDate is 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 selectedDate before calling isDateInDecade is a good improvement. It prevents potential errors when selectedDate is null or undefined.


52-52: LGTM!

The change to adjust the viewDate based on the difference between the year and the year of the selectedDate is a good improvement. It allows for a more dynamic adjustment of the view date, reflecting the selected decade more accurately. The conditional check for selectedDate also 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 selectedDate is 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 value prop. The example enhances the documentation by demonstrating the functionality in practice.

The changes align with the PR objective of introducing the dateValue prop for controlled usage and are consistent with the AI summary.

packages/ui/src/components/Datepicker/Datepicker.stories.tsx (5)

35-63: LGTM!

The ControlledTemplate story is a great addition to demonstrate the controlled behavior of the Datepicker component. The usage of React's useState and useEffect hooks to manage the selected date based on the value prop is implemented correctly. The story also handles the conversion of minDate and maxDate props to Date objects and modifies the defaultValue based on the provided date range, ensuring that the default date falls within the specified limits.


74-79: LGTM!

The Template story is correctly updated to align with the new logic. The handling of defaultDate is changed to defaultValue and it adheres to the same date range constraints.


84-96: LGTM!

The ControlledDefaultEmpty story is a great addition to demonstrate the controlled behavior of the Datepicker component with no selected date. Setting the value prop to null and the label prop to "No date selected" is a good way to showcase this scenario.


111-121: LGTM!

The NullDateValue story is a good addition to demonstrate the behavior of the Datepicker component with a null date value.


123-134: LGTM!

The DateValueSet story is a good addition to demonstrate the behavior of the Datepicker component with a default date value. Setting the defaultValue prop to the current date using new 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 onChange prop, which replaces the previous onSelectedDateChanged prop. The test case logic remains the same and is correct.


83-83: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


99-99: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


116-116: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


133-133: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


150-150: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


170-170: LGTM!

The test case has been correctly updated to use the new defaultValue prop, which replaces the previous defaultDate prop. The test case logic remains the same and is correct.


Line range hint 1-220: LGTM!

The test cases in the Datepicker.spec.tsx file have been correctly updated to use the new prop names:

  • onSelectedDateChanged has been renamed to onChange.
  • defaultDate has been renamed to defaultValue.

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 clear method implementation.


85-101: LGTM!

The changes to the DatepickerProps interface 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 selectedDate and viewDate state variables is correct and handles the controlled and uncontrolled scenarios properly.


Line range hint 146-162: LGTM!

The changes to the changeSelectedDate and clearDate functions are in line with the PR objectives and the AI-generated summary. The modifications ensure that the selected date is updated correctly and the onChange callback is invoked with the updated date.


250-259: LGTM!

The new useEffect hook is necessary to keep the selectedDate state in sync with the controlled value prop. 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 displayValue variable 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 the label prop when no date is selected.


289-292: LGTM!

Updating the viewDate state to selectedDate when 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 value prop of the TextInput component to displayValue ensures that the input field displays the formatted selected date or the label when no date is selected.


296-296: LGTM!

Setting the defaultValue prop of the TextInput component to the formatted initialDate or the label prop ensures that the input field displays the correct initial value when the component is uncontrolled.


359-359: LGTM!

Passing null as the date value to the changeSelectedDate function in the clear button's onClick handler ensures that the selected date is properly cleared when the clear button is clicked.

@ddiasfront
Copy link
Contributor Author

ddiasfront commented Sep 13, 2024

Here.

#1484 (comment)

Alright, done guys, changeset added, shoutout anything else might need.

@rluders @SutuSebastian

rluders
rluders previously approved these changes Sep 13, 2024
@SutuSebastian
Copy link
Collaborator

Here.
#1484 (comment)

Alright, done guys, changeset added, shoutout anything else might need.

@rluders @SutuSebastian

all good 🔥

Copy link
Collaborator

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SutuSebastian, @rluders updated changeset

Copy link
Collaborator

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. The addition of clear functionality (related to Issue DatePicker component defaultDate/clear not working as expected #1039).
  2. 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

📥 Commits

Files that changed from the base of the PR and between dfef19d and f082ca7.

📒 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:

  1. Verify if any additional test files were updated or created, especially for the new controlled input functionality and clear feature.
  2. 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 issue

Verify 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 onSelectedDateChange to onChange). Consider updating the changeset to a minor or major version bump if this breaking change is intended to be included.

@ddiasfront
Copy link
Contributor Author

Thanks for doing this, I mostly just have recommendations on docs changes. It would be helpful if @rluders could review this because he understands this component best, and it's by far our most complex

@tulup-conner can we close this thread and head towards merging ?

@rluders
Copy link
Collaborator

rluders commented Sep 30, 2024

Thanks for doing this, I mostly just have recommendations on docs changes. It would be helpful if @rluders could review this because he understands this component best, and it's by far our most complex

@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.

Copy link
Collaborator

@SutuSebastian SutuSebastian left a comment

Choose a reason for hiding this comment

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

awesome

@SutuSebastian SutuSebastian merged commit 25bb353 into themesberg:main Sep 30, 2024
8 checks passed
@github-actions github-actions bot mentioned this pull request Sep 30, 2024
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.

Datepicker only changes on manual click.
8 participants