Add modules for socket events #1175
Conversation
astorije
added
the
Type: Feature
client/js/socket-events/open.js
Outdated
| @@ -0,0 +1,11 @@ | |||
| "use strict"; | |||
| const $ = require("jquery"); | |||
| const sidebar = $("#sidebar, #footer"); | |||
There was a problem hiding this comment.
I forgot to remove this. Will do so soon.
client/js/socket-events/auth.js
Outdated
| if (token) { | ||
| return; | ||
| } | ||
| sidebar.find(".sign-in") |
There was a problem hiding this comment.
Oh yeah, don't need it, will do so.
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
from
12b838e to
2fb06ca
Compare
|
I rebased this, so it should be ready to go. There were a couple of small things that I could see that were added to the socket events since I did this PR, so I copied them over to the modules. A second pair of eyes to check I didn't miss something would be good. |
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
2 times, most recently
from
12b838e to
2fb06ca
Compare
client/js/utils.js
Outdated
| return array; | ||
| } | ||
|
|
||
| function sortable() { |
There was a problem hiding this comment.
We probably could do. I could do that in another PR? It'll be easier to move things out of util.js than lounge.js in the future.
There was a problem hiding this comment.
Alright, I just did it.
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
from
2fb06ca to
aa4a263
Compare
|
@xPaw Updated |
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
2 times, most recently
from
ddf69e5 to
743e653
Compare
|
Re-rebased again. Could someone else review this so we can get this in before we get more conflicts? ;-) |
|
Uuuuh, sorry...
|
| @@ -0,0 +1,192 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
Any chance you could add a new line between "use strict"; and what comes after in your files?
If this is just too annoying for you, nevermind then, not a big deal :)
There was a problem hiding this comment.
Sure, but if this is something we want to enforce we should probably see if there's an eslint rule (or write our own) so we don't have to point it out.
There was a problem hiding this comment.
I guess http://eslint.org/docs/rules/padding-line-between-statements with explicit stuff for "use strict" would do?
There was a problem hiding this comment.
It doesn't look like "use strict" is one of the options there, but possibly. Let's not do this in this PR anyway, we can do that in another.
client/js/render.js
Outdated
| const sorting = require("./sorting"); | ||
|
|
||
| const chat = $("#chat"); | ||
| const sidebar = $("#sidebar, #footer"); |
There was a problem hiding this comment.
This is probably extracted from lounge.js, but do we actually want to keep both selectors in there?
There was a problem hiding this comment.
Probably not? I'm not sure why it was in it in lounge.js so I'll need to figure that out in the first place before I delete the footer.
There was a problem hiding this comment.
Alrighty, so looking and this is for the buttons in the bottom of the sidebar (add server, help, sign out, etc) which are all still handled in lounge.js, so I have removed it from all the plugins (I checked that it didn't need them in each of them, they all seem to just be doing chan stuff).
| renderChannelMessages, | ||
| renderChannelUsers, | ||
| renderNetworks | ||
| }; |
There was a problem hiding this comment.
Another no-big-deal question: I was under the impression that it was fairly common/standard to add the exports statement at the end of the JS files, am I incorrect?
There was a problem hiding this comment.
So, there are 2 standards, 1 is the add it at the end and 1 is the add it at the beginning.
I tend to add it at the beginning because I like have it front and centre, one of the first things you see when you go in, because it gives you a clear indication what is exported straight away.
|
I haven't done a full review because of the conflict (sorry!) but I did test this locally and so far no issues. |
Are you? Are you really? |
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
from
743e653 to
2abab89
Compare
|
Alright, @astorije I have done everything except removing the "#footer". I'll take a look into that now, but you can review the rest whenever you want. |
AlMcKinlay
force-pushed
the
yamanickill/socket-modules
branch
from
2abab89 to
f90c355
Compare
There was a problem hiding this comment.
It's actually pretty much impossible to fully review this. I don't have the cognitive abilities to memorize 600+ removed lines and see if they're all correctly re-added later 😅
But the modularization looks sane, what I saw in the changes seems fine, and I tested this successfully.
@thelounge/maintainers, make sure to update your instance with latest master after this is merged so we can have this running in multiple places before we release v2.3.2 (probably in a few days).
We can look into/fix the footer/sidebar thingy in a separate PR.
Yeah, I know. But I wasn't making a PR per socket event. That would have been dreadful. Anyway, 🎉 less than 1000 lines in lounge.js now. I'll give it a break before modularising anything else. But I would like to ask that new code doesn't go in lounge.js unless it has to now. |
…odules Add modules for socket events
This one is probably the biggest modularising one. It is quite big, but it's really awkward to split this one down anymore, and it works quite nicely as a chunk together.