Page MenuHomePhabricator

[web] Refactor - remove instances of useMemo + classNames combo
ClosedPublic

Authored by adar on Nov 21 2022, 5:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Nov 7, 5:42 AM
Unknown Object (File)
Thu, Nov 7, 5:42 AM
Unknown Object (File)
Mon, Nov 4, 3:41 PM
Unknown Object (File)
Mon, Nov 4, 3:41 PM
Unknown Object (File)
Mon, Nov 4, 3:41 PM
Unknown Object (File)
Mon, Nov 4, 12:16 PM
Unknown Object (File)
Sat, Oct 26, 9:01 PM
Unknown Object (File)
Sat, Oct 26, 9:01 PM
Subscribers

Details

Summary

Currently in the codebase, we have callsites where we're using the useMemo hook with the classNames library. This is unnecessary as useMemo returns a memoized value, which in this case is just a string and hence safe to run on every render.
https://linear.app/comm/issue/ENG-2287/[web]-do-not-wrap-classnames-in-reactusememo

Test Plan
  • checked for all occurences by git grep 'React.useMemo(() => classNames' -- *.js as well as a VS Code search.
  • started and interacted with the web app to ensure nothing became broken along the way

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

adar edited the test plan for this revision. (Show Details)
adar edited the test plan for this revision. (Show Details)

remove change from setting up dev environment

Please don't add me to reviews except in these cases

Please don't add me to reviews except in these cases

sorry, noted! :)

I *do* want to be subscribed to all diffs, though :)

sorry, noted! :)

No worries, it's your first diff. Congrats on getting it out!

In D5697#169385, @adar wrote:

Please don't add me to reviews except in these cases

sorry, noted! :)

Ah sorry that's my bad, I usually schedule a sync/call going through Phabricator and putting up first diff... which I skipped because you'd worked with Phabricator in the past. Still should've at least sent the Notion docs and explained the norms around adding reviewers and whatnot.

This revision is now accepted and ready to land.Nov 21 2022, 10:10 AM