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: eliminate the lodash.get dependency #964

Merged
merged 3 commits into from
Jan 19, 2025
Merged

feat: eliminate the lodash.get dependency #964

merged 3 commits into from
Jan 19, 2025

Conversation

cheton
Copy link
Member

@cheton cheton commented Jan 18, 2025

PR Type

Enhancement, Dependencies


Description

  • Removed the lodash.get dependency across the codebase.

  • Replaced lodash.get usage with optional chaining and nullish coalescing.

  • Updated package.json to remove lodash.get from dependencies.


Changes walkthrough 📝

Relevant files
Enhancement
base-css.js
Replace `lodash.get` with optional chaining in base CSS   

packages/react/src/css-baseline/base-css.js

  • Removed lodash.get and replaced with optional chaining.
  • Introduced variables for base and mono fonts.
  • Updated CSS to use the new variables.
  • +5/-3     
    styles.js
    Replace `lodash.get` with optional chaining in drawer styles

    packages/react/src/drawer/styles.js

  • Removed lodash.get and replaced with optional chaining.
  • Updated margin calculations to use modern JavaScript features.
  • +2/-3     
    styles.js
    Replace `lodash.get` with optional chaining in modal styles

    packages/react/src/modal/styles.js

  • Removed lodash.get and replaced with optional chaining.
  • Updated margin calculations to use modern JavaScript features.
  • +2/-3     
    Dependencies
    package.json
    Remove `lodash.get` from package dependencies                       

    packages/react/package.json

    • Removed lodash.get from dependencies.
    +0/-1     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link

    codesandbox bot commented Jan 18, 2025

    Review or Edit in CodeSandbox

    Open the branch in Web EditorVS CodeInsiders

    Open Preview

    Copy link

    changeset-bot bot commented Jan 18, 2025

    ⚠️ No Changeset found

    Latest commit: 49c0be9

    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

    @cheton cheton linked an issue Jan 18, 2025 that may be closed by this pull request
    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

    Default Values

    Verify that using 'inherit' as the default font fallback value is the intended behavior for both base and mono fonts

    const baseFonts = theme?.fonts?.['base'] ?? 'inherit';
    const monoFonts = theme?.fonts?.['mono'] ?? 'inherit';
    Null Safety

    Ensure that the optional chaining for sizes and lineHeights objects provides expected results when theme values are undefined

    marginTop: `calc(${sizes?.['4x']} + ${lineHeights?.['xl']} + ${sizes?.['3x']})`,

    Copy link
    Contributor

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

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    ✅ Prevent potential CSS calc() errors by providing fallback values for undefined theme properties
    Suggestion Impact:The commit addressed the same issue but used a different approach - using a pixelize utility function instead of null coalescing operators to handle undefined values

    code diff:

    -      marginTop: `calc(${sizes?.['4x']} + ${lineHeights?.['xl']} + ${sizes?.['3x']})`,
    +      marginTop: `calc(${pixelize(sizes['4x'])} + ${pixelize(lineHeights['xl'])} + ${pixelize(sizes['3x'])})`,

    Add null coalescing operators with default values for sizes and lineHeights to
    prevent potential runtime errors if these theme values are undefined.

    packages/react/src/drawer/styles.js [221]

    -marginTop: `calc(${sizes?.['4x']} + ${lineHeights?.['xl']} + ${sizes?.['3x']})`,
    +marginTop: `calc(${sizes?.['4x'] ?? '0px'} + ${lineHeights?.['xl'] ?? '0px'} + ${sizes?.['3x'] ?? '0px'})`,
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This is an important safety improvement that prevents potential CSS calc() errors when theme values are undefined. Without fallback values, the calc() function could receive 'undefined' values and fail silently.

    8
    General
    Improve null safety and code readability by using destructuring with default values

    Add default values directly in the destructuring to handle cases where theme is
    undefined, making the code more robust and easier to read.

    packages/react/src/css-baseline/base-css.js [4-5]

    -const baseFonts = theme?.fonts?.['base'] ?? 'inherit';
    -const monoFonts = theme?.fonts?.['mono'] ?? 'inherit';
    +const { fonts: { base: baseFonts = 'inherit', mono: monoFonts = 'inherit' } = {} } = theme ?? {};
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion offers a more concise way to handle default values using destructuring, though both approaches (current and suggested) are valid and provide the same null safety. The improvement is mainly stylistic.

    5

    Copy link

    codesandbox-ci bot commented Jan 18, 2025

    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 Jan 18, 2025

    Bundle Report

    Changes will increase total bundle size by 695 bytes (0.02%) ⬆️. This is within the configured threshold ✅

    Detailed changes
    Bundle name Size Change
    @tonic-ui/react-cjs 830.0kB 20 bytes (0.0%) ⬆️
    @tonic-ui/react-esm 778.66kB 675 bytes (0.09%) ⬆️

    Copy link

    codecov bot commented Jan 18, 2025

    Codecov Report

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

    Project coverage is 78.44%. Comparing base (a09e4d9) to head (a000ea7).
    Report is 1 commits behind head on v2.

    Files with missing lines Patch % Lines
    packages/react/src/css-baseline/base-css.js 0.00% 2 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##               v2     #964      +/-   ##
    ==========================================
    - Coverage   78.46%   78.44%   -0.03%     
    ==========================================
      Files         403      403              
      Lines        6692     6694       +2     
    ==========================================
      Hits         5251     5251              
    - Misses       1441     1443       +2     

    ☔ 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 Jan 18, 2025

    Tonic UI Demo

    On 2025-01-18 08:41:44 +0000, PR #964 (a000ea7) was successfully deployed. You can view it at the following link:
    https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-964/

    @cheton cheton merged commit 0ee5860 into v2 Jan 19, 2025
    7 of 8 checks passed
    @cheton cheton deleted the tonic-ui-963 branch January 19, 2025 02:13
    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.

    Eliminate the lodash.get dependency
    2 participants