Page MenuHomePhabricator

[web] Move `ThreadSettingsModal` and related components to new `modals/threads/settings` directory
ClosedPublic

Authored by atul on May 15 2022, 8:00 AM.
Tags
None
Referenced Files
F3364833: D4043.id12687.diff
Mon, Nov 25, 5:15 AM
F3364476: D4043.diff
Mon, Nov 25, 3:56 AM
Unknown Object (File)
Mon, Nov 18, 3:09 PM
Unknown Object (File)
Thu, Nov 14, 10:20 AM
Unknown Object (File)
Sat, Nov 9, 6:36 PM
Unknown Object (File)
Sat, Nov 9, 6:36 PM
Unknown Object (File)
Sat, Nov 9, 5:43 PM
Unknown Object (File)
Sat, Nov 9, 4:58 PM

Details

Summary

web/modals/threads was getting crowded by ThreadSettingsModal-related files... which are going to continue to grow in number.

Created a new settings sub-directory in threads to consolidate the relevant files. Typically prefer to avoid heavily nested directory structure, but I think it makes sense here.

Test Plan

Imports were "fixed" by my IDE/flow/eslint.

Also opened ThreadSettingsModal in browser and clicked around making sure things appeared/worked as expected.

Diff Detail

Repository
rCOMM Comm
Branch
master
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul published this revision for review.May 15 2022, 8:00 AM

cut extraneous .js suffix

My only real comment here is, what do you think about getting rid of the -settings- in many of these file names? If they're in the settings folder now they're kind of redundant, no?

web/modals/threads/notifications/notifications-modal.react.js
26 ↗(On Diff #12687)

nice

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

My only real comment here is, what do you think about getting rid of the -settings- in many of these file names?

Yeah that's a good point, the thread prefix is also probably redundant since the folder structure is like web/modals/threads/settings.. so just like delete-tab.react.js would work?

That said, I still prefer the longer name since I tend to navigate by filename vs directory. Could also see it being confusing 5 years from now when we have a dozen general-tab.react.jss

In D4043#113797, @atul wrote:

My only real comment here is, what do you think about getting rid of the -settings- in many of these file names?

Yeah that's a good point, the thread prefix is also probably redundant since the folder structure is like web/modals/threads/settings.. so just like delete-tab.react.js would work?

That said, I still prefer the longer name since I tend to navigate by filename vs directory. Could also see it being confusing 5 years from now when we have a dozen general-tab.react.jss

My preference is usually to avoid repeated path in file name, but at the same time, as you said, searching by filename is a big advantage of longer names. So it look like keeping the longer names is a better idea.

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