Page MenuHomePhabricator

[web] Use `EnumSettingOption` components in `ThreadSettingsPrivacyTab`
ClosedPublic

Authored by atul on May 15 2022, 1:38 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 28, 2:32 AM
Unknown Object (File)
Fri, Oct 25, 2:59 AM
Unknown Object (File)
Wed, Oct 23, 4:16 PM
Unknown Object (File)
Wed, Oct 23, 4:15 PM
Unknown Object (File)
Wed, Oct 23, 4:15 PM
Unknown Object (File)
Tue, Oct 22, 5:44 AM
Unknown Object (File)
Fri, Oct 18, 5:26 PM
Unknown Object (File)
Wed, Oct 16, 10:14 AM

Details

Summary

This is a minimally-viable first pass.

Replace the inputs in ThreadSettingsPrivacyTab with EnumSettingOptions. There is still a good bit to go with stying, but should be functionally complete.

Here's how it looks:


Depends on D4050

Test Plan

Click around, change visibility, observe that things look/behave as expected

Diff Detail

Repository
rCOMM Comm
Branch
landmay19 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.May 15 2022, 1:40 PM
atul edited the summary of this revision. (Show Details)
atul added inline comments.
web/modals/threads/settings/thread-settings-privacy-tab.react.js
33 ↗(On Diff #12696)

Will probably type these as maybe instead of optional in subsequent diff to avoid having to explicitly write undefined here

114 ↗(On Diff #12696)

Lack of disabled prop is a "known issue," will be addressed later in this stack.

atul added inline comments.
web/modals/threads/settings/thread-settings-privacy-tab.react.js
104–122 ↗(On Diff #12697)

could probably introduce a third "general" callback to cut repetition here

tomek added inline comments.
web/components/enum-settings-option-info.css
5 ↗(On Diff #12697)

Shouldn't it be limited by its parent? It would become more reusable.

web/modals/threads/settings/thread-settings-privacy-tab.react.js
105 ↗(On Diff #12697)

Why do we need to assert a constant?

This revision is now accepted and ready to land.May 17 2022, 7:10 AM

rebase after resolving merge conflicts and before landing

web/components/enum-settings-option-info.css
5 ↗(On Diff #12697)

Yeah, you're right that this is hacky and specifically to get ThreadSettingsPrivacyTab to look right.

Will find a better approach here that's more general

web/modals/threads/settings/thread-settings-privacy-tab.react.js
105 ↗(On Diff #12697)

Good point, there's no longer a reason. We were previously asserting that event.currentTarget.value was a threadType, but that's not longer relevant. Will update.