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
Unknown Object (File)
Sat, May 18, 12:57 AM
Unknown Object (File)
Thu, May 16, 6:13 AM
Unknown Object (File)
Mon, May 6, 5:20 PM
Unknown Object (File)
Fri, Apr 26, 1:45 PM
Unknown Object (File)
Mar 31 2024, 7:02 AM
Unknown Object (File)
Mar 30 2024, 3:27 AM
Unknown Object (File)
Mar 21 2024, 3:05 PM
Unknown Object (File)
Mar 19 2024, 2:16 AM

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

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