Closed PullJosh closed 6 years ago
Haven't yet looked at the code, but this looks awesome.
The code hasn't been changed much. It's mostly just modifying require
locations and breaking App
into separate components.
requireing becomes a bit more verbose With the new system, in which the component lives in a directory of its same name, you need to name the component twice when requireing.
Gonna take a look into resolving this - I've seen browserify plugins for it, I think. Or I can write one.
I've seen browserify plugins for it, I think. Or I can write one.
Woah! I had no idea there was so much you could do with those plugins! (It makes sense, I suppose, but I've never seen it done.)
Best of luck!
:tada:
You can now:
./Component/Component.js
/path/to/decent/packages/client/js/Modal/Modal.js
This one's a biggie. The idea is to organize the client's components in a consistent manner. The big benefit here is that our directory structure now allows components to be nested with a clear hierarchy. I wanted to do this so that working on the two additional tabs of the right sidebar would be a clean and easy process.
This is a pretty massive change, which is why I'm not pushing straight to
preact
. I want your feedback first. With that in mind, here are the pros/cons of the new layout:Pros:
Atom
component is only used byServerPool
, and thatTimeago
is only used inMessage
.App
component no longer houses tons of random details regarding the sidebars or main messages. The main sections of the app (the sidebars and main area) have each been made into their own component, which improves readability a lot. (It's possible that the "join server" modal could also be moved into the left sidebar component, although I decided that it might be out of the scope of this PR. Also, more importantly, I'm lazy.)ComponentName.css
file directly alongside eachComponentName.js
file.require
d through another. (For example, we wouldn't putIcon
as a child ofRightSidebar
because it is also used byApp
, and we don't want torequire('./RightSidebar/Icon/Icon')
fromApp.js
)Cons:
require
ing becomes a bit more verbose With the new system, in which the component lives in a directory of its same name, you need to name the component twice whenrequire
ing. For instance, here is howApp
acquiresRightSidebar
:context
In general, usingthis.context
goes against the classic React method of passing props to children. Pulling from thepool
also means we have to force more manual updates on components. Honestly, this feels like a non-issue to me, but I wanted to include it for completeness.Let me know what you think! Personally, I love the new structure, but wouldn't be upset if anybody disagrees. It's definitely a large change, and I didn't really give any warning before this PR, so a small riot is only acceptable. :stuck_out_tongue: