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

Fix: box border color in tx details #4668

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Fix: box border color in tx details #4668

merged 1 commit into from
Dec 16, 2024

Conversation

katspaugh
Copy link
Member

What it solves

Border color wasn't properly passed to a Box because of the prop order.

I've also added omitBy to exclude unnecessary undefined properties passed to sx.

Screenshot 2024-12-16 at 14 53 11

Copy link

github-actions bot commented Dec 16, 2024

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Code review by ChatGPT

{...props}
/>
)
}
})

Choose a reason for hiding this comment

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

  1. The change to use memo can improve performance by preventing unnecessary re-renders. Ensure all props used deeply are immutable or compare function is provided.
  2. Consider extracting the repeated styles definition code in Box and Typography into a utility function to adhere to the DRY principle.
  3. Ensure that omitBy correctly handles all edge cases when removing undefined values to prevent unintended styling loss.
  4. The import statement export * from '@mui/material/index' can potentially lead to namespace pollution. Consider importing only necessary components.

@usame-algan
Copy link
Member

Can you include a before screenshot please?

Copy link

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page Size (compressed)
global 1016.46 KB (🟡 +135 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Copy link
Member

@usame-algan usame-algan left a comment

Choose a reason for hiding this comment

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

Not sure why omitBy and isUndefined are needed. Is it causing performance issues without?

Copy link

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
73.84% (+0% 🔼)
14383/19478
🔴 Branches 51.39% 3433/6680
🔴 Functions 56.77% 2041/3595
🟡 Lines
75.37% (+0% 🔼)
13043/17306

Test suite run success

1707 tests passing in 231 suites.

Report generated by 🧪jest coverage report action from b0872bb

@katspaugh
Copy link
Member Author

Not sure why omitBy and isUndefined are needed. Is it causing performance issues without?

You can see lots of undefined props passed in the React dev extension.

@katspaugh katspaugh merged commit 2d98a9e into dev Dec 16, 2024
16 checks passed
@katspaugh katspaugh deleted the border branch December 16, 2024 14:04
@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants