[web] Fix thread description and color modification in private thread
ClosedPublic
Actions

Authored by • abosh on May 18 2022, 10:07 PM.

Details

Reviewers

atul

Commits

rCOMM49e364a933bc: [web] Fix thread description and color modification in private thread

Summary

Fix issue where user is unable to modify the thread description and color in their private thread. The thread name is still unmodifiable (following the inclusion of the EDIT_DESCRIPTION and EDIT_COLOR permissions but not the EDIT_THREAD_NAME permission in D1980), but the thread description and color can be modified.

Related Linear issue with full context here.

Test Plan

Tested on Chrome/Safari and works as expected:

Before:

After:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• abosh created this revision.May 18 2022, 10:07 PM

Herald added subscribers: • adrian, tomek, ashoat. · View Herald TranscriptMay 18 2022, 10:07 PM

• abosh edited the summary of this revision. (Show Details)May 18 2022, 10:11 PM

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

• abosh added 1 blocking reviewer(s): atul.

Harbormaster completed remote builds in B9316: Diff 12917.May 18 2022, 10:12 PM

• abosh requested review of this revision.May 18 2022, 10:12 PM

• abosh edited the test plan for this revision. (Show Details)May 18 2022, 10:13 PM

• abosh added inline comments.May 18 2022, 10:16 PM

web/modals/threads/settings/thread-settings-general-tab.react.js
149–152 ↗	(On Diff #12917)	This extra condition makes sure that the thread has the `EDIT_THREAD_NAME` permission. Private threads do not have this permission, so this condition will ensure that the `Input` will be disabled if the thread is a private thread.

• abosh edited the summary of this revision. (Show Details)May 18 2022, 10:18 PM

atul requested changes to this revision.May 19 2022, 6:18 AM

atul added inline comments.

web/modals/threads/settings/thread-settings-general-tab.react.js
149–152 ↗	(On Diff #12917)	Looks good, can we pull this conditional outside of the `disabled` prop? Maybe introduce an `threadNameInputDisabled` variable that gets passed to the `disabled` prop?

This revision now requires changes to proceed.May 19 2022, 6:18 AM

Moved conditional outside disabled prop and memoized the call to threadHasPermission so it's only called once the dependencies changed and not each time the Input is rendered.

Harbormaster completed remote builds in B9361: Diff 12973.May 20 2022, 4:22 AM

Let's "un-memoize" threadNameInputDisabled and then this should be good to land.

web/modals/threads/settings/thread-settings-general-tab.react.js
136–139 ↗	(On Diff #12973)	We don't need to memoize this since `threadHasPermission` returns a boolean. We typically use `React.useMemo` when we're getting an object (eg array, function, etc) and want to preserve referential equality. Might also make sense to memoize something that's computationally expensive, but I don't think `threadHasPermission` is?

This revision now requires changes to proceed.May 20 2022, 7:50 AM

Got it. I assumed we needed to memoize it since I was only looking at the dependency (threadInfo) but didn't realize threadNameInputDisabled only returns a boolean which is much less expensive to compute in this case.

Harbormaster completed remote builds in B9371: Diff 12987.May 20 2022, 12:03 PM

atul accepted this revision.May 20 2022, 12:04 PM

atul added inline comments.

web/modals/threads/settings/thread-settings-general-tab.react.js
152 ↗	(On Diff #12987)	We should probably rename this in another diff to something like `threadSettingsOperationInProgress` or something to make it more descriptive.