-
Notifications
You must be signed in to change notification settings - Fork 865
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(ux): move preferences to menubar #1425
Conversation
816117d
to
229dac8
Compare
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.
Nice 😊 Suggested some text tweaks and also one question. In your screenshot, it looks like both shortcuts are enabled, but the "Enable ..." menu items are still visible. Should they say "Disable ..." in that scenario? I'm a little unclear how the user disables these.
assets/locales/en.json
Outdated
"ipfsOnPathInstall": { | ||
"title": "Install IPFS on PATH", | ||
"message": "By enabling this option, IPFS will be available on your command line as \"ipfs\". This action is reversible.", | ||
"action": "Install" |
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.
For both the dialog boxes (and their menu items) related to IPFS on PATH, we're using "install/enable" and "uninstall/disable" interchangeably, which is a little confusing. Let's decide on either install/uninstall or enable/disable and be consistent throughout ... I might go for enable/disable so that it feels more like an option rather than something more severe, but there might be precedence for install/uninstall elsewhere that I'm unaware of?
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.
Just updated everywhere to enable/disable that is not related to IPFS Desktop updates 😃
I might go for enable/disable so that it feels more like an option rather than something more severe
Agreed. We're not actually installing anything but when updating or enabling npm on IPFS
, which I will make more clear.
@jessicaschilling thanks for pointing that out. They’re actually not enabled. Those options enable them globally. They are checkboxes. Now that I think of it, being a checkbox and having a verb (“Enable...”/“Disable...”) does not seem to be a great idea. Either we keep the checkbox and remove the verb (simpler solution) or we toggle between verbs (Enable and Disable). Checkbox tick example: Here's an example of a mac app I use (Lungo) whose preferences are just checkboxes: |
Checkboxes are good! If unchecked, do “take screenshot” and “disable hash” menu items act accordingly (do the shortcuts disappear)? |
The shortcuts always work if the menu is open. That’s why they’re visible. The preferences enable them globally, ie, even with the menu closer, you can press them and they’ll work. I guess we should find a better way to pass that message. |
OH ... got it. Sorry! Good point though; I was confused, others will be too. We don’t actually need “Enable” in menu items - the checkboxes imply it (see your Lungo example). Removing it would allow room for “Global Screenshot Shortcut” and “Global Download Shortcut”, which might be more explicit. Would like to run that past @lidel though. |
Yes, I agree we should wait for @lidel opinion on this 😃 |
1ca47ca
to
869c28c
Compare
@hacdias - I'd be inclined to leave these under "Advanced" ... to a certain degree, they're sort of applicable to being considered preferences ... but only to advanced users. ;) |
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.
Labels look okay. Would keep "Advanced" as-is for now.
Small concern below tho.
@@ -16,7 +16,7 @@ | |||
"readReleaseNotes": "Read Release Notes", | |||
"status": "Status", | |||
"files": "Files", | |||
"settings": "Settings", |
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.
Removing this from the top level menu may be a problem. I believe users will look for the "WebUI Settings" screen under "Settings" and won't find any entry point there.
Can we add it at the very top of new "Settings" submenu, to maintain continuity, of sorts?
One entry followed by a separator and then existing entries under "Desktop" header (similar to existing "Experiments"):
@jessicaschilling Does this make sense? If so, what would be the correct wording here: "Open Node Settings"? "Open WebUI Settings"?
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.
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.
This is great. Agree on “Node Settings” over “WebUI settings sine the thing you’re adjusting is the node itself.
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.
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.
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.
Having a clarification of “these are the prefs for this app” is a good idea. I might just say “App Preferences” though? Not sure if “desktop integrations” makes sense unless you’re already deeply familiar.
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.
@lidel what do you think?
869c28c
to
69a1f6c
Compare
bb8a92c
to
f66bd70
Compare
@lidel other than the the 'Desktop Integrations' label, is there anything you'd change? |
f66bd70
to
c704288
Compare
LGTM, but let's get @lidel's blessing first 😊 |
Yes! |
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
daa6fac
to
1d289a9
Compare
License: MIT Signed-off-by: Henrique Dias <hacdias@gmail.com>
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.
LGTM, thanks! (no strong feeling about labels, just wanted to have the option in menu 👍)
Feel free to merge if nothing else is needed here.
* chore: remove old event handler removed in #1425 * refactor: use ipcMainEvents constant
This PR removes the Desktop settings from the Web UI panel! Related to #1389.
License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com