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/checkbox,react/radio): use useEffectOnceWhen hook to display warning message in non-production environments #937

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

cheton
Copy link
Member

@cheton cheton commented Oct 8, 2024

PR Type

enhancement


Description

  • Introduced the useEffectOnceWhen hook in both Checkbox and Radio components to handle warning messages.
  • Removed the isNameConflictRef logic and replaced it with a warningMessage variable to streamline warning message management.
  • Ensured that warning messages about name conflicts are displayed only in non-production environments.

Changes walkthrough 📝

Relevant files
Enhancement
Checkbox.js
Use `useEffectOnceWhen` for warning messages in Checkbox 

packages/react/src/checkbox/Checkbox.js

  • Added useEffectOnceWhen hook to manage warning messages.
  • Removed isNameConflictRef and replaced with warningMessage logic.
  • Display warning messages only in non-production environments.
  • +12/-12 
    Radio.js
    Use `useEffectOnceWhen` for warning messages in Radio       

    packages/react/src/radio/Radio.js

  • Added useEffectOnceWhen hook to manage warning messages.
  • Removed isNameConflictRef and replaced with warningMessage logic.
  • Display warning messages only in non-production environments.
  • +12/-11 

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    codesandbox bot commented Oct 8, 2024

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    changeset-bot bot commented Oct 8, 2024

    ⚠️ No Changeset found

    Latest commit: 516f57d

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Performance Concern
    The useEffectOnceWhen hook is used with a dependency array that includes a boolean expression. This might cause unnecessary re-renders if the expression changes frequently.

    Performance Concern
    Similar to the Checkbox component, the useEffectOnceWhen hook is used with a dependency array that includes a boolean expression, which might cause unnecessary re-renders.

    Code Duplication
    The warning message logic is duplicated in both Checkbox and Radio components. Consider extracting this logic into a shared utility function.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Encapsulate warning logic in a custom hook for better code organization and reusability

    Consider using a custom hook to encapsulate the warning logic for both Checkbox and
    Radio components, promoting code reuse and maintainability.

    packages/react/src/checkbox/Checkbox.js [77-82]

    -useEffectOnceWhen(() => {
    -  if (process.env.NODE_ENV !== 'production' && !!warningMessage) {
    -    // Log the warning message only once
    -    console.error(warningMessage);
    -  }
    -}, [!!warningMessage]);
    +useWarningEffect(warningMessage);
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This suggestion promotes code reuse and maintainability by encapsulating the warning logic into a custom hook, which can be beneficial if the logic is used across multiple components.

    7
    Simplify property assignments using modern JavaScript operators

    Consider using optional chaining and nullish coalescing operators to simplify the
    assignment of disabled and name properties.

    packages/react/src/radio/Radio.js [57-62]

    -disabled = (disabled ?? radioGroupDisabled);
    -name = (name ?? radioGroupName);
    +disabled ??= radioGroupDisabled;
    +name ??= radioGroupName;
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The use of modern JavaScript operators like nullish coalescing can simplify the code and improve readability, making it a worthwhile enhancement.

    6
    Best practice
    Use a constant for environment checks to improve code maintainability

    Consider using a constant for the 'production' environment check to improve
    maintainability and reduce the risk of typos.

    packages/react/src/checkbox/Checkbox.js [78]

    -if (process.env.NODE_ENV !== 'production' && !!warningMessage) {
    +const isDevEnvironment = process.env.NODE_ENV !== 'production';
    +if (isDevEnvironment && !!warningMessage) {
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Using a constant for environment checks can reduce the risk of typos and improve maintainability, but the impact is relatively minor as the current code is already clear.

    5
    Maintainability
    Improve code readability by using descriptive variable names for boolean conditions

    Consider using a more descriptive variable name for the boolean condition in the
    useEffectOnceWhen hook's dependency array.

    packages/react/src/checkbox/Checkbox.js [77-82]

    +const hasWarningMessage = !!warningMessage;
     useEffectOnceWhen(() => {
    -  if (process.env.NODE_ENV !== 'production' && !!warningMessage) {
    +  if (process.env.NODE_ENV !== 'production' && hasWarningMessage) {
         // Log the warning message only once
         console.error(warningMessage);
       }
    -}, [!!warningMessage]);
    +}, [hasWarningMessage]);
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: While using a more descriptive variable name can slightly improve readability, the impact is minimal as the existing code is already understandable.

    4

    💡 Need additional feedback ? start a PR chat

    Copy link

    codesandbox-ci bot commented Oct 8, 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 Oct 8, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 77.80%. Comparing base (5116834) to head (516f57d).
    Report is 1 commits behind head on v2.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##               v2     #937      +/-   ##
    ==========================================
    - Coverage   77.80%   77.80%   -0.01%     
    ==========================================
      Files         403      403              
      Lines        6632     6636       +4     
    ==========================================
    + Hits         5160     5163       +3     
    - Misses       1472     1473       +1     

    ☔ 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 Oct 8, 2024

    Tonic UI Demo

    On 2024-10-08 13:49:26 +0000, PR #937 (516f57d) was successfully deployed. You can view it at the following link:
    https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-937/

    …play warning message in non-production environments
    @cheton cheton merged commit 7b994c5 into v2 Oct 10, 2024
    7 checks passed
    @cheton cheton deleted the tonic-ui-927c branch October 10, 2024 00:36
    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