Page MenuHomePhabricator

[web] Rename `NotificationsOption` -> `EnumSettingsOption`
ClosedPublic

Authored by atul on May 15 2022, 9:44 AM.
Tags
None
Referenced Files
F1786173: D4045.id12763.diff
Sat, May 18, 3:46 PM
F1786172: D4045.id12689.diff
Sat, May 18, 3:46 PM
F1786171: D4045.id12762.diff
Sat, May 18, 3:46 PM
F1786170: D4045.id12688.diff
Sat, May 18, 3:45 PM
F1786163: D4045.id.diff
Sat, May 18, 3:45 PM
F1786160: D4045.diff
Sat, May 18, 3:44 PM
F1783732: D4045.id12689.diff
Sat, May 18, 11:38 AM
Unknown Object (File)
Fri, May 17, 5:03 AM

Details

Summary

We want to reuse the (previously known as) NotificationsOption component for the "Privacy" tab of ThreadSettingsModal.

As a first step, we rename NotificationsOption to more generic EnumSettingsOption (open to alternative names). In later diffs will remove "Notifications-specific" parts of EnumSettingsOption and make it more generic.

Made sure to rename component, rename file, and fix imports.


Depends on D4044

Test Plan

Opened NotificationsModal and it continues to look/work as expected. Also flow/eslint.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cut extraneous .js suffix

This revision is now accepted and ready to land.May 16 2022, 6:04 AM
This revision now requires review to proceed.May 16 2022, 6:14 AM

Not sure what will be the responsibility of these components after the refactoring, but we can always find a better name after its done. From what I understand now, the component will handle any setting, not only notification-specific. Why do you think we should start the name with enum? It sounds like SettingsOption should work ok for a generic component, but it is possible I'm missing something.

This revision is now accepted and ready to land.May 16 2022, 9:54 AM

Why do you think we should start the name with enum? It sounds like SettingsOption should work ok for a generic component, but it is possible I'm missing something.

I couldn't think of a great name at the moment so went with Enum to match some of the names (of divs/css selectors) that were already in the ThreadSettingsPrivacyTab component.

I think SettingsOption might be too generic? But still very open to suggestions since I agree it's a bit of an odd name... could add a diff at end of stack with rename if we decide on one