Page MenuHomePhabricator

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

Authored by atul on May 15 2022, 9:44 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 8, 4:20 PM
Unknown Object (File)
Thu, Nov 28, 8:56 PM
Unknown Object (File)
Thu, Nov 28, 8:01 PM
Unknown Object (File)
Thu, Nov 28, 7:16 PM
Unknown Object (File)
Nov 25 2024, 4:35 AM
Unknown Object (File)
Nov 25 2024, 2:42 AM
Unknown Object (File)
Nov 15 2024, 6:11 AM
Unknown Object (File)
Nov 9 2024, 10:40 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