Page MenuHomePhabricator

[web] Move `changeQueued` up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`
ClosedPublic

Authored by atul on Apr 18 2022, 1:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 5, 5:07 AM
Unknown Object (File)
Sun, Nov 3, 1:26 PM
Unknown Object (File)
Sun, Nov 3, 1:26 PM
Unknown Object (File)
Sun, Nov 3, 1:26 PM
Unknown Object (File)
Mon, Oct 28, 2:33 AM
Unknown Object (File)
Thu, Oct 10, 11:01 AM
Unknown Object (File)
Thu, Oct 10, 3:24 AM
Unknown Object (File)
Thu, Oct 10, 3:24 AM

Details

Summary

Move changeQueued out from inner class component to outer "Connected" functional component.

(Also refactored the logic to simplify things/avoid lodash)

As part of the work to refactor ThreadSettingsModal... specifically to turn it into a functional component


Depends on D3762

Test Plan
  1. Made changes in ThreadSettingsModal
  2. Logged value of changeQueued and made sure it was as expected

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/modals/threads/thread-settings-modal.react.js
438–442 ↗(On Diff #11569)

Experimented to make sure that this logic works as expected.

f959.png (246×1 px, 55 KB)

Adding @ashoat as first pass reviewer since he initially wrote the logic I refactored

atul requested review of this revision.Apr 18 2022, 1:52 PM

further simplify changeQueued logic with .values

val => v... looks better?

I don't recall anything about this logic. Feel free to add me back if you think there is some unique insight I am able to provide

tomek added inline comments.
web/modals/threads/thread-settings-modal.react.js
447–448 ↗(On Diff #11644)

A combination of filter and length > 0 can be expressed by using some. At this point we could even use something like .some(Boolean) but it is not that maintainable - introducing a change that sets a boolean might cause a hard to catch bug.

This revision is now accepted and ready to land.Apr 21 2022, 2:52 AM

rebase before addressing feedback

web/modals/threads/thread-settings-modal.react.js
447–448 ↗(On Diff #11644)

Oh nice, didn't know about some, looks cleaner