Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces two new SVG icon components, MergeOutlinedIcon and SettingsRemoteIcon, and exports them from the main icons index. Feedback suggests optimizing these components by wrapping them in React.memo to prevent unnecessary re-renders and adding displayName for better debugging. There is also a suggestion to review the naming convention to ensure consistency with existing icons by potentially removing the Icon suffix.
| export const MergeOutlinedIcon: React.FC<SvgIconProps> = (props) => { | ||
| return ( | ||
| <SvgIcon {...props}> | ||
| <path d="M6.41 21 5 19.59l4.83-4.83c.75-.75 1.17-1.77 1.17-2.83v-5.1L9.41 8.41 8 7l4-4 4 4-1.41 1.41L13 6.83v5.1c0 1.06.42 2.08 1.17 2.83L19 19.59 17.59 21 12 15.41z" /> | ||
| </SvgIcon> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
For SVG icons in a React application, it is a best practice to wrap the component in React.memo to prevent unnecessary re-renders when the parent component updates. Additionally, adding a displayName is helpful for debugging in React DevTools.
Also, consider if the Icon suffix is necessary for consistency. While some icons in this repository use it, the majority (e.g., Search, Settings, Share) do not.
| export const MergeOutlinedIcon: React.FC<SvgIconProps> = (props) => { | |
| return ( | |
| <SvgIcon {...props}> | |
| <path d="M6.41 21 5 19.59l4.83-4.83c.75-.75 1.17-1.77 1.17-2.83v-5.1L9.41 8.41 8 7l4-4 4 4-1.41 1.41L13 6.83v5.1c0 1.06.42 2.08 1.17 2.83L19 19.59 17.59 21 12 15.41z" /> | |
| </SvgIcon> | |
| ); | |
| }; | |
| export const MergeOutlinedIcon = React.memo((props: SvgIconProps) => ( | |
| <SvgIcon {...props}> | |
| <path d="M6.41 21 5 19.59l4.83-4.83c.75-.75 1.17-1.77 1.17-2.83v-5.1L9.41 8.41 8 7l4-4 4 4-1.41 1.41L13 6.83v5.1c0 1.06.42 2.08 1.17 2.83L19 19.59 17.59 21 12 15.41z" /> | |
| </SvgIcon> | |
| )); | |
| MergeOutlinedIcon.displayName = 'MergeOutlinedIcon'; |
| export const SettingsRemoteIcon: React.FC<SvgIconProps> = (props) => { | ||
| return ( | ||
| <SvgIcon {...props}> | ||
| <path d="M15 9H9c-.55 0-1 .45-1 1v12c0 .55.45 1 1 1h6c.55 0 1-.45 1-1V10c0-.55-.45-1-1-1m-3 6c-1.1 0-2-.9-2-2s.9-2 2-2 2 .9 2 2-.9 2-2 2M7.05 6.05l1.41 1.41C9.37 6.56 10.62 6 12 6s2.63.56 3.54 1.46l1.41-1.41C15.68 4.78 13.93 4 12 4s-3.68.78-4.95 2.05M12 0C8.96 0 6.21 1.23 4.22 3.22l1.41 1.41C7.26 3.01 9.51 2 12 2s4.74 1.01 6.36 2.64l1.41-1.41C17.79 1.23 15.04 0 12 0" /> | ||
| </SvgIcon> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Similar to other icon components, wrapping this in React.memo will improve performance by avoiding redundant re-renders. Explicitly setting the displayName also improves the debugging experience.
Regarding naming, since a Settings icon already exists without the Icon suffix, naming this SettingsRemote would be more consistent with the overall project structure.
| export const SettingsRemoteIcon: React.FC<SvgIconProps> = (props) => { | |
| return ( | |
| <SvgIcon {...props}> | |
| <path d="M15 9H9c-.55 0-1 .45-1 1v12c0 .55.45 1 1 1h6c.55 0 1-.45 1-1V10c0-.55-.45-1-1-1m-3 6c-1.1 0-2-.9-2-2s.9-2 2-2 2 .9 2 2-.9 2-2 2M7.05 6.05l1.41 1.41C9.37 6.56 10.62 6 12 6s2.63.56 3.54 1.46l1.41-1.41C15.68 4.78 13.93 4 12 4s-3.68.78-4.95 2.05M12 0C8.96 0 6.21 1.23 4.22 3.22l1.41 1.41C7.26 3.01 9.51 2 12 2s4.74 1.01 6.36 2.64l1.41-1.41C17.79 1.23 15.04 0 12 0" /> | |
| </SvgIcon> | |
| ); | |
| }; | |
| export const SettingsRemoteIcon = React.memo((props: SvgIconProps) => ( | |
| <SvgIcon {...props}> | |
| <path d="M15 9H9c-.55 0-1 .45-1 1v12c0 .55.45 1 1 1h6c.55 0 1-.45 1-1V10c0-.55-.45-1-1-1m-3 6c-1.1 0-2-.9-2-2s.9-2 2-2 2 .9 2 2-.9 2-2 2M7.05 6.05l1.41 1.41C9.37 6.56 10.62 6 12 6s2.63.56 3.54 1.46l1.41-1.41C15.68 4.78 13.93 4 12 4s-3.68.78-4.95 2.05M12 0C8.96 0 6.21 1.23 4.22 3.22l1.41 1.41C7.26 3.01 9.51 2 12 2s4.74 1.01 6.36 2.64l1.41-1.41C17.79 1.23 15.04 0 12 0" /> | |
| </SvgIcon> | |
| )); | |
| SettingsRemoteIcon.displayName = 'SettingsRemoteIcon'; |
Signed-off-by: Prarthana <226818177+prarii@users.noreply.github.com>
5c1349a to
28119ba
Compare
Notes for Reviewers
This PR fixes #1418
Signed commits