-
Notifications
You must be signed in to change notification settings - Fork 864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: just a menubar #866
feat: just a menubar #866
Conversation
@ipfs-shipyard/gui I need y'all here 😄 This is a big change! |
On Linux in order for changes made to individual MenuItems to take effect, you have to call setContextMenu again. Without this, updateStatus(status) in src/lib/tray.js does not update the menu. License: MIT Signed-off-by: Marcin Rataj <lidel@lidel.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Menu did not update on Linux (i3bar
), but I've found a fix (see cab400c) and it displays updated menu correctly on right-click now:
Not sure why left-click does not work, perhaps it is i3bar
I use for tray.
(this should not block this PR, we can investigate this later).
Quick thoughts on menu itself:
- Settings should be moved out of "Advanced" to the top level, and be in the same order as in Web UI (under "Peers"), to minimize user confusion :)
- "Check for updates" could be moved to "Advanced" submenu to declutter UI a bit
- Overall, I like it a lot! This change makes it feel light and snappy.
I think what I just pushed might fix that.
Yas. Talking about that: we only have three options in settings related to Desktop. Perhaps they could be checkbox-type buttons on a Preferences sub-menu where the user could activate them and deactivate them.
Yeah!
Feels light, but it isn't light... In the future I'd like to investigate possible ways of moving out of Electron. It would be lighter and perhaps even/actually more native. Perhaps native implementations for each OS with a C/C++/Go shared library. Thinking too much? After this is closes, I'll certainly open an issue about the nativeness of our app and what we can do to make it better. Or perhaps native apps that rely on our Node library which must be possible too. |
I really like it. I think we could drop the third divider. something like
And what about adding a link to docs.ipfs.io? this'd draw more attention to it and encourage us to add a section on Desktop and Web UI. |
also of note, the status link isn't working for me. |
@olizilla will restructure that way! Seems a good idea.
Neither for me, thanks for noticing. |
@olizilla I don't think so, especially when you want to start the node again: It's pretty odd finding that function buried, and having the |
Yeah, I agree with those two. |
And perhaps we could hide the versions behind 'About IPFS Desktop'. Instead of opening the repository, we could show a simple informational dialog. |
Hmm yeah, if that dialog is customisable we can have the links to the repos there! |
@fsdiogo I was following the example of Docker for Mac. I don't find it weird, but I don't feel strongly about it. I don't think "Stop IPFS" should be in the main menu. A user has to restart IPFS to make a config change, but we should offer that option in the UI, when they make a config change. In the meantime we could add a "Restart IPFS" option to either the advanced, or the main menu if you feel strongly that it should be there. My assumption is that for most users, we'd like them to be able to set their node up once (ideally just run with the defaults) and then leave IPFS running. If IPFS takes too much resources, we should limit it. We just dropped the number of open connections it'll try and make from 600+ to about 150, which should help. |
@lidel how do you feel about this? |
@olizilla the idea of restarting through Web UI is great 👌 I feel strongly that it shouldn't be in the advanced, and we should have the option to @hacdias as we're talking about Docker, this |
|
@fsdiogo I really like Docker's about page. Although remember: that's a native window. We can't do one as native as that one. What we could do is create a very simple web page to show. Again - it would be really nice if in the feature we could have really native apps for each platform. @lidel I like the idea of starting/stopping and restart being under the status item. I agree with dropping 'Open' and 'View' labels. I personally don't like having those options under 'About'. It feels weird to me. Another interesting thing is that on macOS we usually see 'Per Word Capitalisation' and on Windows we have 'Per sentence capitalisation'. |
I think we are zooming in on an ideal list. I like @lidel's proposal, pulling the start and restart under the status seems neat. In general think the menu items should be things you can do, so verbs rather than nouns.
|
If you can't put stop and restart under a submenu, then just inline them.
|
|
||
### IPFS daemon always running | ||
|
||
IPFS Desktop's main feature is to allow you to have the IPFS daemon always running in the background. But fear not! If you need to stop it, you can do it under the 'Advanced' options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
middle=$(($3 / 2)) | ||
center=$((middle - 1)) | ||
|
||
convert -size "$3x$3" xc:transparent \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very cool
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not for this PR, but would be good to document the dependencies for running this script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do later :) I can even just add a comment there or add to the README. There's something I don't really like about that: not sure if the sizes are perfect. They look good but it seems that they're not as correct as they should be. But we can take a look at that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}, | ||
{ | ||
label: i18n.t('openConfigFile'), | ||
click: () => { shell.openItem(store.path) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about exposing the IPFS Desktop config file to users. It's confusing that it isn't related to the much more documented IPFS config.
I'm not gonna block the PR for this, I just want to flag that my expectation here was that it'd open the IPFS config file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we already had that possibility. It might be useful for someone who wants to change the flags that we use to run the daemon.
const updateStatus = data => { | ||
status = data | ||
|
||
menu.getMenuItemById('ipfsIsStarting').visible = status.starting && !status.done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about just setting the status to one of these string values in the first place? with the collection of booleans, it's harder to be sure that we have covered every eventuality. If status can only be 1 string, then we can quickly scan the deamon init code to see all the possible values, and we can just use it directly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I wanted to make those constants and just compare. I'll do that in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be even better to just change the text and the icon... But this is not native and Electron doesn't let us do that.
@olizilla I'll update the circle in another PR and the other things you've mentioned! |
w00t! ship it! |
For the record: |
I've taken a bit of time today to work on this since I really enjoyed the idea - and I liked the result very much. Don't get mistaken by the 'wip' in the title. The only thing missing are changes to the README 😄The software is workiiiiiing!
It is working on macOS and Windows. It is supposed to work better on Linux now since we don't have the issue of which 'screen' is showing (some envs would just show the menubar with options on either click).
Please read #865 for further background information. @ericronne suggested to collapse the Web UI options (from Status to Peers) into one 'Open Web UI'. Although I actually like the current configuration. Please try it out!
TODO