Related Linear issue here. Had this in my git stash from last night doing some quick and easy Backlog work, putting up the diff now.
Details
Details
Tested the ColorSelectorModal before and after and it remained the same and the ColorSelector worked as expected.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/chat/settings/color-selector-modal.react.js | ||
|---|---|---|
| 100 ↗ | (On Diff #15836) | Noticed that this could be memoized (see lines 93-98). |
| native/chat/settings/color-selector-modal.react.js | ||
|---|---|---|
| 54 ↗ | (On Diff #15836) | Most of the functions should be wrapped in useCallback so that they don't change on each render. This one is even simpler, as we can just assign the correct value to it. |
| 58 ↗ | (On Diff #15836) | In functional component it might be more convenient to destructure props. It's not necessary, but you can consider it. |
| 85–88 ↗ | (On Diff #15836) | Have you tested this code thoroughly? useMemo takes a function as a parameter and stores its return value - in this case undefined because we're not returning explicitly. I bet it wasn't the intention. |
Comment Actions
Address @tomek's feedback. Added useCallback to memoize functions and destructured props.
| native/chat/settings/color-selector-modal.react.js | ||
|---|---|---|
| 81 ↗ | (On Diff #15985) | Please make this dependency list as specific as possible. Depending on props would mean that every time any prop changes we recompute the value. |
| 102–104 ↗ | (On Diff #15985) | When the only purpose of a lambda is to return a value, you can use a shorthand syntax |
Comment Actions
Destructure changeThreadSettings so editColor doesn't recompute each time props changes. I think the reason I was confused before is because we also import changeThreadSettings from lib/actions/thread-actions (line 9) but it is also passed in as a prop with the same name.
Also remove { } and return in one line return statement.