Add tabs for teams - Githubissues

geekytime commented 8 years ago

Sorry this is so big, but it took a fair amount of refactoring to get this to work cleanly. The good news is that this adds tabs for teams, with full support for notifications per tab, and basic support for themes (so if your teams are different colors, the tab background honors that).

I also had some minor adventures with the webpack/livereload situation that were getting in the way, and sort of needed to be dealt with so I could work at speed.

I think my commits tell the story pretty well, so that would be a good place to start reviewing.

The "app state" concept feels a little tricky to follow for folks who are new to React. If you're interested in the back-story behind this approach, read Matti Lankinen's article Goodbye Flux, Welcome Bacon/RX?.

The app-state module could definitely use some refactoring to simplify the pattern and reduce the amount of boilerplate needed per variable/action, but I wanted to watch the pattern emerge a bit more before abstracting it away, so I don't code myself into a corner.

geekytime commented 8 years ago

I forgot that I also added support for Chrome command-line switches in an early commit, here.

geekytime commented 8 years ago

Also note, this implementation is deliberately a bit Spartan on the presentation, as it's really only needed until Mattermost implements its own tab view.

ninja- commented 8 years ago

until Mattermost implements its own tab view.

from my point of view it should stay within matterfront just as slack does, because the accounts aren't related(or on different servers), have different preferences set etc. I like the way you implemented teams in this PR. Also, in future mattermost should add it's own minimal integration api so getting notification data wouldn't look hacky(hooking into webview still but straight to register listeners)

geekytime commented 8 years ago

from my point of view it should stay within matterfront just as slack does, because the accounts aren't related(or on different servers), have different preferences set etc

Agreed. This is a dirt-simple way to support people who need teams from multiple servers.

in future mattermost should add it's own minimal integration api so getting notification data wouldn't look hacky

I totally agree. The DOM-scraping we used here is definitely a last resort, and should be replaced with an integration api for notifications whenever it becomes available. I toyed with the idea of using the existing integrations, but they are per-channel right now in Mattermost, which would just be a mess.

ninja- commented 8 years ago

integration api(what we want to get):

team name, url, connection status, icon (? first time I noticed there are no team icons like in slack)
unread counts for every channel and sum (easy)

integration_api.jsx(an empty react component or initialised on boot):

an event emitter
registers to ChannelStore and TeamStore
abstracts hooks and provides functions like registerTeamListener, registerChannelListener (channel listeners is passing channel objects and unread counts)
API to steal notifications (seriously, it might be useful later on + node-notifier) http://www.tutorialsavvy.com/2015/07/electron-desktop-notification-using-node-notifier.html/
available via window.mm_integration

Simple design, stable enough API. Simple enough to be implemented in few minutes, but I doubt it would be merged soon enough to depend on it here(to mattermost). At least I created a ready spec though.

lloeki commented 8 years ago

I'll have some time next week to review that.

ninja- commented 8 years ago

@geekytime I have some problems running your version from npm run start

App threw an error when running [TypeError: Cannot convert undefined or null to object]
A JavaScript error occurred in the main process
Uncaught Exception:
TypeError: Cannot convert undefined or null to object
    at Function.keys (native)
    at Object.chromeArgs.apply (/matterfront/src/chrome-args.js:7:10)
    at Object.<anonymous> (/matterfront/src/main.js:10:12)

the code to read settings should be something like this I think?

  var chromeArgs = settings.get('chrome-args') || {};

then the build wouldn't work without I think without less and babel-loader which aren't in package.json?

then webpack couldn't compile css

ERROR in Loader matterfront/node_modules/less/index.js didn't return a function
 @ ./src/browser/index.less 4:14-115
(for every file)

which results in this on "electron ."

Uncaught Error: Cannot find module "!!./../../node_modules/css-loader/index.js!./../../node_modules/less/index.js!./team-buttons.less"

EDIT: ok so another missing package was less-loader....

The configuration loader is really buggy...

now crashed on TeamButton's team.name being null where team equals

bject
mentionCount: 0
themeData: Object
unreadCount: 0
url: "https://pre-release.mattermost.com"
__proto__: Object

I can't get it to work because teams are lacking their names everywhere

geekytime commented 8 years ago

the code to read settings should be something like this I think? var chromeArgs = settings.get('chrome-args') || {};

Yeah, that's probably right. I need those chrome-args there for my network config to work, so I probably didn't test very well without it.

geekytime commented 8 years ago

then the build wouldn't work without I think without less and babel-loader which aren't in package.json?

I'll delete my node_modules folder and start fresh. Sorry if I missed saving a few dependencies. We probably need to get our travis build set up to catch these kinds of things, so it doesn't take another person wasting time to tell us when we miss them.

geekytime commented 8 years ago

I can't get it to work because teams are lacking their names everywhere

I can't figure out how to recreate this one. I'll dig into this one a bit more after I solve the other two.

You do have unique team names in your state.json file, right? I assume you do, but I figured it's worth checking.

geekytime commented 8 years ago

then the build wouldn't work without I think without less and babel-loader which aren't in package.json?

I removed my node_modules folder and everything works fine after an npm install. And the package.json does seem to have less, babel-loader, and less-loader in it. I'm not sure what's going on there, but it might be something with your npm config? You could try clearing your npm cache, maybe.

geekytime commented 8 years ago

the code to read settings should be something like this I think? var chromeArgs = settings.get('chrome-args') || {};

Yeah, that's probably right. I need those chrome-args there for my network config to work, so I probably didn't test very well without it.

I fixed this one by adding empty defaults for the chrome-args property in the config.

LongLiveCHIEF commented 8 years ago

:+1:

LongLiveCHIEF commented 8 years ago

@geekytime do you want to merge this?

geekytime commented 8 years ago

@geekytime do you want to merge this?

I do, but I also want to make sure it works for people before we merge. Have the issues above been resolved, @ninja- ?

geekytime commented 8 years ago

@H3Chief, does this branch definitely work on your machine?

LongLiveCHIEF commented 8 years ago

Nope, I get a blank white screen with a vertical grey bar on the left side. I also get a deprecation warning, and an error in the console:

ipc module is deprecated. Use require("electron").ipcRenderer instead
//
Uncaught TypeError: Cannot read property 'substr' of undefined

I think the first is actually a regression, because I remember patching and committing that fix. It also shows that the package.versions is 1.1.0 when we've already released v1.2.0. So there was definitely a regression introduced somewhere.

LongLiveCHIEF commented 8 years ago

Error mentioned above seems to be originating from /src/browser/team-button.jsx#L7.

ninja- commented 8 years ago

Yes @h3chief that's team.name == null everywhere

LongLiveCHIEF commented 8 years ago

looks like we're also missing the chokidar dev dependency

ninja- commented 8 years ago

@H3Chief @geekytime guys, @yuya-oc has made some big progress on https://github.com/yuya-oc/electron-mattermost lately, including proper tab support, he's also developing the UI with react. we might consider moving the work there?

LongLiveCHIEF commented 8 years ago

@ninja- as far as I can tell, we're just about exactly the same (counting this PR), with the exception that he has a UI to add the teams, and we have backend support for chrome flags (proxies and such).

geekytime commented 8 years ago

@H3Chief was able to reproduce some of these issues on his Mac, and opened another PR in our fork to address them. (I'm still not sure why it was working on my machine. Oh well.)

I'll test his code tomorrow morning, and hopefully we can get this branch fixed and merged shortly after that.

lloeki commented 8 years ago

Well I finally tested this. I admit I'm way over my head with React which I never used, so I couldn't do a proper review (BTW thanks for the pointer to the article, it was a good read).

Anyway, this is working great and while spartan, quite nice with a tab count > 1 (hence #74). Thanks to all of you!

lloeki / matterfront

Add tabs for teams #67