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(react/date-pickers)!: deprecate Calendar and replace it with DateCalendar #961

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

cheton
Copy link
Member

@cheton cheton commented Dec 21, 2024

No description provided.

Copy link

codesandbox bot commented Dec 21, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

Copy link

changeset-bot bot commented Dec 21, 2024

🦋 Changeset detected

Latest commit: e6ca5ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@tonic-ui/react Minor

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

Copy link

codecov bot commented Dec 21, 2024

Bundle Report

Changes will increase total bundle size by 5.22kB (0.19%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
@tonic-ui/react-cjs 832.64kB 2.64kB (0.32%) ⬆️
@tonic-ui/react-esm 781.24kB 2.58kB (0.33%) ⬆️

Copy link

codesandbox-ci bot commented Dec 21, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (0ee5860) to head (e6ca5ff).
Report is 1 commits behind head on v2.

Files with missing lines Patch % Lines
...eact/src/date-pickers/DateCalendar/DateCalendar.js 86.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##               v2     #961      +/-   ##
==========================================
+ Coverage   78.44%   78.66%   +0.21%     
==========================================
  Files         403      404       +1     
  Lines        6694     6706      +12     
==========================================
+ Hits         5251     5275      +24     
+ Misses       1443     1431      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@trendmicro-frontend-bot
Copy link
Contributor

trendmicro-frontend-bot commented Dec 21, 2024

Tonic UI Demo

On 2025-01-20 03:03:11 +0000, PR #961 (e6ca5ff) was successfully deployed. You can view it at the following link:
https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-961/

@cheton cheton marked this pull request as ready for review January 14, 2025 02:07
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Duplication

The deprecation warning logic and prop mapping could be moved to a separate utility function to avoid code duplication between DateCalendar and Calendar components

{ // deprecation warning
  const prefix = `${DateCalendar.displayName}:`;

  useOnceWhen(() => {
    warnDeprecatedProps('date', {
      prefix,
      alternative: 'value',
      willRemove: true,
    });
  }, (dateProp !== undefined));

  useOnceWhen(() => {
    warnDeprecatedProps('defaultDate', {
      prefix,
      alternative: 'defaultValue',
      willRemove: true,
    });
  }, (defaultDateProp !== undefined));

  // TODO: remove `date` and `defaultDate` props in next major version
  valueProp = valueProp ?? dateProp;
  defaultValueProp = defaultValueProp ?? defaultDateProp;
}
Naming Consistency

The callback handlers onDateCalendarChange and onDateCalendarError should be renamed to match the component prop names onChange and onError for better consistency

const onDateCalendarChange = useCallback((nextDate) => {
  const isControlled = (valueProp !== undefined);
  if (!isControlled) {
    setValue(nextDate);
  }

  if (isValid(nextDate)) {
    setInputValue(format(nextDate, inputFormat));
  }

  if (typeof onChangeProp === 'function') {
    onChangeProp(nextDate);
  }

  if (closeOnSelect) {
    onClose();
  }
}, [valueProp, inputFormat, onChangeProp, closeOnSelect, onClose]);

const onDateCalendarError = useCallback((error, value) => {
  setError(error);
  if (typeof onErrorProp === 'function') {
    onErrorProp(error, value);
  }
}, [onErrorProp]);

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add date validation before triggering the onChange callback to prevent invalid dates from being set

The onChange callback should validate the date before calling onChangeProp to ensure
data consistency. Add validation before the callback.

packages/react/src/date-pickers/DateCalendar/DateCalendar.js [190-197]

 const onChange = useCallback((nextDate) => {
+  const validationError = validateDate(nextDate, minDate, maxDate, shouldDisableDate);
+  if (validationError) {
+    onErrorProp?.(validationError, nextDate);
+    return;
+  }
   const isControlled = (valueProp !== undefined);
   if (!isControlled) {
     setDate(nextDate);
   }
   onChangeProp?.(nextDate);
-}, [valueProp, onChangeProp]);
+}, [valueProp, onChangeProp, minDate, maxDate, shouldDisableDate, onErrorProp]);
  • Apply this suggestion
Suggestion importance[1-10]: 8

Why: The suggestion adds important validation logic to prevent invalid dates from being propagated through the onChange callback, which could prevent bugs and improve data consistency.

8
Add missing dependencies to useEffect to ensure proper validation when min/max dates change

The useEffect dependency array for date validation is missing minDate and maxDate
dependencies, which could cause stale validations.

packages/react/src/date-pickers/DateCalendar/DateCalendar.js [148-154]

 useEffect(() => {
   const validationError = validateDate(date, minDate, maxDate, shouldDisableDate);
   if (validationError !== previousValidationError) {
     onErrorProp?.(validationError, date);
   }
-}, [date, onErrorProp, previousValidationError, validationError]);
+}, [date, minDate, maxDate, shouldDisableDate, onErrorProp, previousValidationError, validationError]);
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: Adding missing dependencies (minDate, maxDate, shouldDisableDate) to the useEffect dependency array is important to ensure date validation is re-run when these constraints change.

7

Copy link
Contributor

codiumai-pr-agent-free bot commented Jan 18, 2025

CI Failure Feedback 🧐

(Checks updated until commit 7154f64)

Action: deploy

Failed stage: Deploy to gh-pages [❌]

Failure summary:

The action failed because of a Git push error to the gh-pages branch. The remote repository contains
changes that are not present in the local repository. This is typically caused by:

  • Another concurrent push to the same branch
  • Local repository being out of sync with remote
  • Missing git pull to integrate remote changes before pushing

  • Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    276:  ./styled-system/pseudo-style-props.html
    277:  ./404.html
    278:  ./theme.html
    279:  ./images/
    280:  ./images/Trend-Micro-Logo.svg
    281:  ./images/tonic-logo-dark.svg
    282:  ./images/patterns/
    283:  ./images/patterns/notification/
    284:  ./images/patterns/notification/icon-notification-error.svg
    ...
    
    314:  ./_next/static/chunks/pages/customization/
    315:  ./_next/static/chunks/pages/customization/shadow-dom-bc965c799ef5106c.js
    316:  ./_next/static/chunks/pages/customization/css-theme-variables-c47c40c5d112c4f8.js
    317:  ./_next/static/chunks/pages/customization/content-security-policy-ef65efa369d9d11c.js
    318:  ./_next/static/chunks/pages/styled-system/
    319:  ./_next/static/chunks/pages/styled-system/style-props-3c37492f523a1309.js
    320:  ./_next/static/chunks/pages/styled-system/responsive-values-2204b04b3d14c3b0.js
    321:  ./_next/static/chunks/pages/styled-system/pseudo-style-props-b986e5bd6e653a45.js
    322:  ./_next/static/chunks/pages/_error-70f1843b45b2e935.js
    ...
    
    669:  148 files changed, 576 insertions(+), 576 deletions(-)
    670:  rename react/pr-961/_next/static/{VnOsivWAbXzdv1UsrozIS => 9DeAicJEFjIV7vzZhwr5c}/_buildManifest.js (99%)
    671:  rename react/pr-961/_next/static/{VnOsivWAbXzdv1UsrozIS => 9DeAicJEFjIV7vzZhwr5c}/_ssgManifest.js (100%)
    672:  rename react/pr-961/_next/static/chunks/{main-93b6710e429c3f1e.js => main-83643afc6015b320.js} (99%)
    673:  rename react/pr-961/_next/static/chunks/pages/{_app-01339a8e141e3e7b.js => _app-bf80a8a46ec95ed6.js} (99%)
    674:  rename react/pr-961/_next/static/chunks/pages/customization/{shadow-dom-69e155771c2ba3e9.js => shadow-dom-bc965c799ef5106c.js} (95%)
    675:  To github.com:trendmicro-frontend/tonic-ui-demo.git
    676:  ! [rejected]          gh-pages -> gh-pages (fetch first)
    677:  error: failed to push some refs to 'github.com:trendmicro-frontend/tonic-ui-demo.git'
    678:  hint: Updates were rejected because the remote contains work that you do not
    679:  hint: have locally. This is usually caused by another repository pushing to
    680:  hint: the same ref. If you want to integrate the remote changes, use
    681:  hint: 'git pull' before pushing again.
    682:  hint: See the 'Note about fast-forwards' in 'git push --help' for details.
    683:  ##[error]Process completed with exit code 1.
    

    ✨ CI feedback usage guide:

    The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
    The tool analyzes the failed checks and provides several feedbacks:

    • Failed stage
    • Failed test name
    • Failure summary
    • Relevant error logs

    In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

    /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"
    

    where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

    Configuration options

    • enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
    • excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
    • enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
    • persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
    • final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

    See more information about the checks tool in the docs.

    BREAKING CHANGE: The `Calendar` component has been deprecated. Use `DateCalendar` component instead.
    @cheton cheton force-pushed the feat/date-calendar branch from 7154f64 to c74d334 Compare January 19, 2025 14:11
    @cheton cheton merged commit 56f957c into v2 Jan 20, 2025
    8 checks passed
    @cheton cheton deleted the feat/date-calendar branch January 20, 2025 03:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants