Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@spartDev
Copy link
Contributor

@spartDev spartDev commented Apr 11, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Release notes for users (delete if codebase-only change)

  • Create a desktop app vesrion of spectrum

closes: #257

Copy link
Contributor

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great start already!

The main big change I (with no Electron experience) can see here is that we really want the frontend to be bundled into the Electron app, rather than loaded as a remote URL, right?

@spartDev
Copy link
Contributor Author

Yes I think loaded as a remote URL it's a good idea, because the app is universal and we do not want to ship node server with the app imo

@mxstbr
Copy link
Contributor

mxstbr commented Apr 11, 2018

Yes I think loaded as a remote URL it's a good idea, because the app is universal and we do not want to ship node server with the app imo

We don't need to ship a Node server with the app if we bundle the frontend into Electron, since it'll just query api.spectrum.chat, right?

@spartDev
Copy link
Contributor Author

Yes my bad you totally right

@spartDev
Copy link
Contributor Author

We start having something !

For now I have created a packager script to ship only for mac
You can test it by running:

yarn electron:package-mac

then go to build-electron folder and launch Spectrum.app

I'm facing an issue right now (open the dev tools):
screen shot 2018-04-11 at 20 09 54

@spartDev spartDev changed the title configure Electron dev environement Desktop app Apr 11, 2018
@spartDev
Copy link
Contributor Author

problem solved, we need to disable Electron's Node integration.
electron/electron#1753

make code smart

remove useless package

use the prod optimized build when not in dev mode

optimze electron dev

create packager script for mac

add electron app icons

disabled nodeIntegration

add packager script for linux

add packager script for windows

create electron app
@spartDev
Copy link
Contributor Author

I rebase my PR for more more clarity.
For now we have:

  • A preload script to create a “bridge” between Electron and the remote spectrum web app.
  • A badge count for notifications
  • A splash screen

I still have to work on the autoupdater.

@mxstbr
Copy link
Contributor

mxstbr commented Apr 13, 2018

Amazing—I'll test this locally!

@mxstbr
Copy link
Contributor

mxstbr commented Apr 14, 2018

I still have to work on the autoupdater.

This will require us to host some sort of server, right? I heard some folks are using GitHub releases for that purpose, is that an option for us?

@spartDev
Copy link
Contributor Author

We can use GitHub Releases, Amazon S3, DigitalOcean Spaces or generic HTTP(s) server). I think GitHub Releases is a good option for us. Maybe @nikgraf who seems to have a very good xp in electron can help us on this ?

@spartDev
Copy link
Contributor Author

OK the real things begins !!

I rename the folder from electron to desktop for more consistency.
I use electron-builder who is a complete solution to package, build & publish electron app.

I had to move previous scripts from package.json to desktop/package.json because electron-builder have a small conflict with CRA, both use a buildfolder.

The only thing I do not like is that the package.json file in desktop folder need to have a name and version set (mandatory by electron-builder). It's redundant but I did not find better ...
If anyone has a better idea !!

You can test now.

@spartDev
Copy link
Contributor Author

Oh the last thing to do is application code signing

@spartDev
Copy link
Contributor Author

I have fix several bug on dev environment, fix the splash screen who did not appear and create an about dialog.

screen shot 2018-04-17 at 00 25 40

@spectrum-bot
Copy link

spectrum-bot bot commented May 15, 2018

Fails
🚫

These new files do not have Flow enabled:

  • desktop/src/autoUpdate.js
  • desktop/src/config.js
  • desktop/src/main.js
  • desktop/src/menu.js
  • desktop/src/preload.js
Warnings
⚠️

These modified files do not have Flow enabled:

  • shared/notification-to-text.js

Generated by 🚫 dangerJS

@mxstbr
Copy link
Contributor

mxstbr commented May 28, 2018

I'm tackling push notifications, let's get this ready to be beta tested! ✌️

mxstbr added 3 commits May 28, 2018 15:59
My initial idea was to use our existing NavBar component, which is
already listening to new notifications, and to show push notifications
from there. That was way more complex than necessary honestly, so I just
manually listen to the subscriptions in `src/index.js` and it's working
super well.

This now shows push notifications for all desktop users! 🎉
@mxstbr
Copy link
Contributor

mxstbr commented May 28, 2018

The latest commits implement push notifications in the desktop app! 🎉

TODO BEFORE MERGING

  • Don't send push notifications if the user's looking at the app
  • Fix Linux and Windows builds (they currently fail with some icon error)
  • Figure out all configuration options necessary for the best experience
  • Set min-width to 770px to avoid people going into the mobile view
  • Set min-height to 600px (or maybe even smaller) for smaller screens
  • Fix isElectron check to not trigger in Cypress
  • Implement hasReply into push notifications Not possible as we dispatch push notifications from the renderer process, not the main process.

@mxstbr
Copy link
Contributor

mxstbr commented May 29, 2018

I think this is pretty much ready! @brianlovin mind giving this a last code review, then doing a prod push with the frontend changes of this branch?

As soon as those are live, anybody who has a build of the desktop app should start seeing push notifications (if the app is open) 🎉

@mxstbr mxstbr mentioned this pull request May 29, 2018
3 tasks
@brianlovin
Copy link
Contributor

Prod cut incoming!

@brianlovin
Copy link
Contributor

Thanks so much @spartDev - this has been awesome to use so far

@brianlovin brianlovin merged commit 50288b7 into withspectrum:alpha May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop app

6 participants