-
Notifications
You must be signed in to change notification settings - Fork 324
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
Feature: ON / OFF Toggle #500
Conversation
- OFF state disables all redirects and removes API client - things that don't require access to API still work: linkify, copy IPFS Path, custom protocol handlers (they just redirect to public gw) - removes window.ipfs (firefox) or throws error on access (chrome) - moves status to the top - cleanups Preferences screen, removes unused fields when Embedded node type is picked - changes fonts to ones from ipfs-css
@lidel this is rad. How about instead of removing items that won't work currently we gray them out with a title attr like "Please enable IPFS Companion". I think it'll feel more stable if the menu remains the same height, but items fade in and out of a based on the state. That might reduce any issue caused by the resize events during initial render too. I kinda like the stats in the top box, but now the menu footer feels a little "unconnected" I don't quite know the right words to explain that feeling, but with the stats at the bottom the box feels a little more solid / grounded. Maybe I'm wrong, what do you think? |
@olizilla I think keeping and fading-out status items is indeed a better way, just added a commit that does just that: We still have to resize popup due to context-dependent menu items, but that is below the static header, so should be ok for now. I want to keep this PR small and release to Beta for further feedback, as many people were interested in "OFF switch". Things for separate PRs:
|
Yep, that is better. How about including the menu options, I think it'll look calmer if the box doesn't resize. |
Hey, I think it’s great to have an enable/disable for companion and given the requests for the feature there’s definitely an argument for having it accessible from the drop down and not just in the options page. I have a few concerns I just wanted to raise:
|
On Sun, 17 Jun 2018, 09:24 Alan Shaw, ***@***.***> wrote:
Hey, I think it’s great to have an enable/disable for companion and given
the requests for the feature there’s definitely an argument for having it
accessible from the drop down and not just in the options page. I have a
few concerns I just wanted to raise:
- naming - I’m not sure if there’s a good answer but could users
confuse on/off with actually starting/stopping the ipfs node?
This is a quick way for a user to disable all of the things companion does,
without forcing them to disable it in the extention settings, where they
are more likely to forget to turn it back on, as it's that removes it from
the toolbar buttons (in chrome at least)
- behaviour - if running an embedded node what happens - is it
actually stopped? Should the behaviour be more similar for both node types
i.e. don’t actually stop an embedded node?
It stops what companion is doing. If it's an embedded node, it stops that
too.
- importance - is on/off our number 1 feature? It certainly looks that
way atm. Perhaps a smaller toggle in the top right/left might be better? We
don’t *really* want people to disable companion, right(?), but it
sounds as though it might be necessary
It gives new users a quick way to see the difference that companion makes
over regular we browsing, so it gets top billing. For existing users it's
still a feature you want quick access to.
- sanity double check - is disabling the add-on in the browser
preferences effectively the same thing?
Similar, but it keeps the extension in view.
- embedded node relegation - I understand the argument that js-ipfs
has issues atm wrt stability/reliability but relegating this feature to
(only) the options screen is going to hugely impact it’s exposure to users
which in turn won’t help us make it more stable/reliable. Would adding
warning messages to clearly state that it’s experimental suffice?
The next part of this work is to add a button right after the on/off switch
that let's the use select which ipfs backend they want to use. This'll give
is space to give the right messaging around js-ipfs
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#500 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADl97-7kjBFswSxrqJSCsKmZqQuXho5ks5t9hI0gaJpZM4UpT7E>
.
|
By which I mean, they are all totally legit concerns, but I think it's
gonna be ok. Right now I'm finding the enable/disable toggle in the
extension settings page to be my most used feature, so it'll help me at
least.
There is more detail on "where are we going with this" in
#491
We only want to relegate js-ipfs a little bit, and put the option on it's
own page where we can let the user add extra ones and add one for the new
built in go-ipfs node in Brave, so the node toggle was gonna get a little
overloaded.
|
@olizilla It indeed may make things more calm, but may introduce some dead entries. (RED) ones will be inactive unless user is on
@alanshaw Good question. Would something like "Active" / "Suspended" communicate the intent better?
I agree we should de-emphasize the toggle. Moving it to the corner is a viable option. |
@olizilla just pushed version that does not hide inactive menu items, but hides context-items in non-ipfs contexts: Non-IPFS ContextIPFS Context |
Do we want to move the toggle before this PR is merged? BottomTop CornerPersonally I like top right corner better as it does not add any height to the UI, making it more compact. |
+1 |
Top right looks neat, but I think that's partly because the labels have been removed. I feel it should have a label. I'd either leave it in the middle, or go top right an pull the label into the switch http://www.uiparade.com/portfolio/toggle-switch-2/ |
@olizilla I really want to avoid label, as it may bring too much focus to the toggle. If material switch is too ambiguous, what if we replace it with ipfs-css/icons/glyph_power.svg? Quick mockup (glyph version, but stroke one may be a better choice): Another option is to move icon back to the middle, but utilize the space better, by adding other quick actions, such as quick access to Preferences: |
I like it! The last one with the power and config icons in the middle is my favourite. |
+1 yes, do it |
- added icons for on/off and preferences - fading status items to indicate issues with API More: #500 (comment)
🔥 HAWT Just to check, does the cog open preferences? Do we need a menu item in additon to the cog? |
Yes, it opens Preferences. We can remove it, but if we keep it it may be easier to onboard different types of people (some people are more visual, others default to text). As an additional data point, this is what user will see when extension is enabled, but IPFS API is down (eg. they did not install go-ipfs yet): I wonder if we should de-colourize icons when API is down (make them white/ |
When the "power is off" I'd make the power button slightly brighter than the disabled options. Right now it looks like it might also be disabled. Perhaps make the power icon svg be I'd drop the "open preferences" option. Let's make the cog the way to get to the extension settings. I think we need to do more thinking on the "API is down" state, but that's for another PR. Generally tho, this is looking great 👍 ✨ |
Thanks! When in "power off' state, icons look more actionable now: As suggested, dropped duplicate "Open Preferences" from the menu to decrease clutter. I've been thinking about using yellow/red accent color to indicate API being down when in "power on" state, but for now let's go with simple fade-out. |
TL;DR
Background
As noted in #491, multiple users raised the need for "off" switch:
Another issue: a toggle between External (js-ipfs-api) and Embedded (js-ipfs) node types in the main UI:
Changes
This PR delivers an initial part of #491:
window.ipfs
linkify, copy IPFS Path, custom protocol handlers
(they just redirect to public gw)
Open Questions
(We had issues with pop-up in past, would be useful to review and confirm by a Mac user)