Page MenuHomePhabricator

[web] Push "Delete thread" functionality down to `ThreadSettingsDeleteTab`
ClosedPublic

Authored by atul on Apr 25 2022, 10:34 AM.
Tags
None
Referenced Files
F3353047: D3837.diff
Sat, Nov 23, 7:51 AM
Unknown Object (File)
Tue, Nov 19, 2:59 AM
Unknown Object (File)
Tue, Nov 19, 2:13 AM
Unknown Object (File)
Mon, Nov 18, 11:56 PM
Unknown Object (File)
Thu, Nov 7, 10:16 PM
Unknown Object (File)
Thu, Nov 7, 9:47 PM
Unknown Object (File)
Thu, Nov 7, 7:39 PM
Unknown Object (File)
Tue, Nov 5, 4:56 AM

Details

Summary

Push "Delete thread" functionality (onDelete & deleteThreadAction) down to ThreadSettingsDeleteTab component.

Also made necessary "supporting changes":

  • Move "Delete" button to ThreadSettingsDeleteTab
  • Pass additional props to ThreadSettingsDeleteTab so deleteThreadAction and onDelete could be "recreated" from within the child component

(This component is definitely in need of restyling, but will sequence that a bit later in ThreadSettingsModal work)


Depends on D3832

Test Plan
  1. Open ThreadSettingsModal
  2. Navigate to the "Delete" tab
  3. Make sure that everything continues to look as expected

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul requested review of this revision.Apr 25 2022, 10:53 AM
web/modals/threads/thread-settings-modal.react.js
226 ↗(On Diff #11883)

Going to move the "button" for General and Privacy tab into their respective Tab components in subsequent diff

This does mean we may have to "repeat ourselves" with changeThreadSettingsAction in two components, but I think that's a okay tradeoff and leads to overall cleaner abstraction

tomek added inline comments.
web/modals/threads/thread-settings-modal.react.js
226 ↗(On Diff #11883)

To avoid the repetition, we probably can create a new component that contains the logic and is used on these both cards

This revision is now accepted and ready to land.Apr 26 2022, 8:47 AM
benschac added inline comments.
web/modals/threads/thread-settings-delete-tab.react.js
52 ↗(On Diff #11952)

@atul I think this should be unknown?

web/modals/threads/thread-settings-delete-tab.react.js
52 ↗(On Diff #11952)

Yes! Thanks for catching that. Going to do a quick sweep through the other strings in ThreadSettingsModal and the children Tab components to make sure there aren't any other typos.