fix: [M3-7174] - Fix BarPercent console errors and other improvements#9716
Conversation
bnussman-akamai
requested review from
carrillo-erik and
jdamore-linode
and removed request for
a team
| } = props; | ||
|
|
||
| return ( | ||
| <StyledDiv className={`${className}`}> |
There was a problem hiding this comment.
If className was undefined, this would actually write undefined to the DOM. This probably wasn't intended
| const StyledLinearProgress = styled(LinearProgress, { | ||
| label: 'StyledLinearProgress', | ||
| shouldForwardProp: (prop) => isPropValid(['rounded'], prop), | ||
| shouldForwardProp: (prop) => isPropValid(['rounded', 'narrow'], prop), |
There was a problem hiding this comment.
This fixes the main console error
mjac0bs
left a comment
mjac0bs
left a comment
There was a problem hiding this comment.
Confirmed VM12523:106 Warning: Received truefor a non-boolean attributenarrow. is no longer shown as a console warning for narrow BarPercent components. Confirmed no "undefined" in the console. Also confirmed that there were no unexpected regressions or console warnings for the BarPercents in the MNTP dialog or Linode Details > Network tab.
I did notice a few console warnings unrelated to BarPercent (mostly caused by NotificationSection) when testing this PR by turning on the mocks and checking the notification menu, so I created a follow up tech story ticket for that: M3-7180.
abailly-akamai
left a comment
abailly-akamai
left a comment
There was a problem hiding this comment.
- no more console logs ✅
- imports and storybook ✅
| ); | ||
| }; | ||
|
|
||
| export default React.memo(BarPercent); |
There was a problem hiding this comment.
I guess it may be to trivial to remove the memoization if we assume the value changing often but curious to know your reasons. Those are micro optimisations but they add up in the end.
There was a problem hiding this comment.
My primary reason for omitting it is just because I figured it was a micro optimisation. I'd be cool with adding it back.
I generally don't know the absolute best practice for cases like this. Here's the React docs about it https://react.dev/reference/react/memo#should-you-add-memo-everywhere
What would you reccomend in this situation?
There was a problem hiding this comment.
The way i think about it (for React.memo specifically) is (will prob paraphrase a thousand sources):
"Is it more expensive to re-render or to compare props". If it re-renders all the time, then running a prop diff is worse than letting the component re-render every time cause we're adding overhead.
In this case i would keep it (it does not mean you have to, it's rather subjective here) cause in a lot of cases this component's props won't change that often so I don't think it hurts to prevent unnecessary re-rendering. The component is small and the performance impact is completely negligible by itself, but in the grand scheme of things every bit helps I guess.
There was a problem hiding this comment.
That makes sense to me. Thank you for the explanation! I'll re-add it and merge!
4 participants
Description 📝
BarPercentcomponent 🛠️Major Changes 🔄
narrowprop getting passed to the underlying componentundefinedclassNamePreview 📷
How to test 🧪
undefinedclassName