Conversation
|
Hi! I've done this kind of thing before. I unfortunately this feature, and not finished. Your version looks cool! But will it run on OS Windows? |
ifedapoolarewaju
left a comment
ifedapoolarewaju
left a comment
There was a problem hiding this comment.
Sorry for the delay in reviewing this. I have dropped some comments/questions. Please let me know what you think.
| "pack": "gulp build && build --dir", | ||
| "dist": "gulp build && build -mwl --x64" | ||
| "pack": "electron-builder --dir", | ||
| "dist": "electron-builder" |
There was a problem hiding this comment.
could you explain the reason for this change? Why are we now running the command this way?
| "snap", | ||
| "AppImage", | ||
| "zip", | ||
| "deb" |
There was a problem hiding this comment.
what is the reason for removing the build config?
|
|
||
| app.on('ready', () => { | ||
| createWindow(); | ||
| createTray(); |
There was a problem hiding this comment.
this functionality should probably only apply to a windows machine
|
|
||
| mainWindow.on('minimize', function (event) { | ||
| event.preventDefault(); | ||
| mainWindow.hide(); |
There was a problem hiding this comment.
this functionality should probably only apply to a windows machine. Also, what is the difference between "minimize" and "hide"? 🤔
|
|
||
| mainWindow.on('closed', () => mainWindow = null) | ||
| mainWindow.on('close', function (event) { | ||
| if (!app.isQuiting) { |
There was a problem hiding this comment.
I think we can just use this clause instead, or not?
|
@ifedapoolarewaju Hello, please mind that this is just a sketchy pull request and it could had been an enhancement request with some code. I've only tested it on debian stable (i3-gaps latest, polybar latest) and it works fine. I did some modifications to package.json for my own testing purposes (more specifically I had issues with gulp and hence I tried to avoid it) and I also removed some targets from the building process in order to speed up my tests. Of course master's configs won't need any change on an upcoming merge. I also added this dependency "electron-json-config" (yes I know that adding a dependency just for 2 lines of code hurts but I may be useful later, although it depends on the design and the features it wants to provide) to save the color of the icon the user had before exiting, cause I didn't want to mess with cookies. I'm pretty sure the pro version uses something similar for the dark theme. If you are into the idea, please let me know, so that I can remove any conflicts from the configs. But you have to take some decisions : 1) Which icons will be used for the notification tray (I used the ones that where shipped with the app but iirc I did some simple conversions and transformations on them)? 2) How user's preferences will be retrieved? |
|
@rand0x0m so this tray support applies to windows and linux systems? |
|
@ifedapoolarewaju Yes it does. Sorry for the late reply but I've been really busy lately. |
ifedapoolarewaju
force-pushed
the
master
branch
5 times, most recently
from
3f2ddfb to
6b32139
Compare
|
Isn't the tray thing slowly passing out? 🤔 |
ifedapoolarewaju
force-pushed
the
master
branch
9 times, most recently
from
c614180 to
f09f454
Compare
4 participants
This is more like a blueprint. It is not ready to merge. Decisions about the system tray image should be taken. And package.json should be changed accordingly. I built it for personal use and it works fine with some custom icons.
It would be nice to save user preference on 'light' or 'dark' icon on exit. A design decision should be taken here.
Lastly, shutdown hook needs to be checked again.