Page MenuHomePhabricator

[web] Change search button clickable area
ClosedPublic

Authored by patryk on Jul 6 2023, 5:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 11, 12:24 PM
Unknown Object (File)
Thu, Apr 11, 9:52 AM
Unknown Object (File)
Thu, Apr 11, 9:52 AM
Unknown Object (File)
Thu, Apr 11, 9:52 AM
Unknown Object (File)
Mar 27 2024, 11:54 PM
Unknown Object (File)
Mar 6 2024, 8:47 AM
Unknown Object (File)
Feb 26 2024, 6:22 PM
Unknown Object (File)
Feb 26 2024, 6:22 PM
Subscribers

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Jul 7 2023, 1:46 AM

It is usually a good idea for a parent to position their children instead of children positioning themselves. Concretely, we should prefer to set gap on flex container instead of setting margin on arbitrarily chosen component.

web/chat/thread-top-bar.css
52–63 ↗(On Diff #28433)
web/chat/thread-top-bar.react.js
90 ↗(On Diff #28433)

This line will cause a new array to be created on every render, which may cause performance issues, so it's always better to avoid it. There are a couple of techniques which can be used to avoid the recomputation:

  1. Instead of creating an array directly, we can place it inside useMemo hook.
  2. Sometimes the array doesn't depend on anything (props / state / etc.). In those cases it can be created outside a component and used as a constant.
  3. When it comes to class names, there's a library classnames which can be used. It produces a string which is safe to use as a prop.

But none of these should be used here, because after introducing gap, this component will have only one class.

This revision now requires changes to proceed.Jul 7 2023, 1:46 AM

Looks good! This diff is simple so it doesn't matter that much, but usually we're including a screenshot of the result in a test plan / summary.

This revision is now accepted and ready to land.Jul 10 2023, 3:47 AM