[web] Move up `ComponentDid[Mount/Update]` functionality to `ConnectedThreadSettingsModal` `useEffect`
ClosedPublic
Actions

Authored by atul on Apr 20 2022, 3:57 PM.

Details

Reviewers

• jacek
tomek
ashoat

Commits

rCOMMd4415ad9efa5: [web] Move up `ComponentDid[Mount/Update]` functionality to…

Summary

Move the componentDidMount and componentDidUpdate lifecycle handlers to a useEffect in the outer "Connected" component.

As part of the work to refactor ThreadSettingsModal... specifically to turn it into a functional component (any day now...)

Depends on D3796

Test Plan

Haven't tested yet, will need to set up multiple users/clients to test

Diff Detail

Repository

rCOMM Comm

Branch

actuallylandapril21 (branched from master)

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 20 2022, 3:57 PM

Herald added subscribers: • abosh, • benschac, • karol and 2 others. · View Herald TranscriptApr 20 2022, 3:57 PM

atul retitled this revision from [web] Move up `ComponentDid[Mount/Update]` to `ConnectedThreadSettingsModal` `useEffect` to [web] Move up `ComponentDid[Mount/Update]` functionality to `ConnectedThreadSettingsModal` `useEffect`.Apr 20 2022, 3:57 PM

atul edited the summary of this revision. (Show Details)

atul added a parent revision: D3796: [web] Move `onSubmit` up from `ThreadSettingsModal` to `ConnectedThreadSettingsModal`.

atul added inline comments.Apr 20 2022, 4:02 PM

web/modals/threads/thread-settings-modal.react.js
124–139 ↗	(On Diff #11683)	My high level understanding of what's happening here is: If the user is on the "delete" tab and the user no longer has permissions for the "delete" tab, then the `currentTabType` should be set back to "general." I don't fully understand why we need to access the previous props to achieve this? I feel like knowing what tab the user is on and whether they have permissions for the respective tab is all that's needed?
458–459 ↗	(On Diff #11683)	see: https://phabricator.ashoat.com/D3783#104491

Harbormaster completed remote builds in B8371: Diff 11683.Apr 20 2022, 4:02 PM

atul requested review of this revision.Apr 20 2022, 4:02 PM

This makes sense, but I think @ashoat should also take a look

web/modals/threads/thread-settings-modal.react.js
124–139 ↗	(On Diff #11683)	It seems like this handles the case where a user opened the tab and then the permission was revoked. But this condition doesn't prevent a user from opening the tab. We show the delete tab only when `canDeleteThread` is true, so it shouldn't be possible to open it without the permission. So overall it looks like this logic can be generalized by removing the check on prev props - which you've done.
462–467 ↗	(On Diff #11683)	This can be even more general

ashoat accepted this revision.Apr 22 2022, 6:01 AM

ashoat added inline comments.

web/modals/threads/thread-settings-modal.react.js
124–139 ↗	(On Diff #11683)	We're already accessing previous props for `this.props.currentTabType`, so we don't go in an infinite loops repeatedly setting the tab. Given that, it's probably safe to remove the `prevPermissionForDeleteTab` check.