Skip to content
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

Merged
merged 10 commits into from
Jun 20, 2018
Merged

Feature: ON / OFF Toggle #500

merged 10 commits into from
Jun 20, 2018

Conversation

lidel
Copy link
Member

@lidel lidel commented Jun 15, 2018

TL;DR

wait, we had no on/off switch before? 🤔

peek 2018-06-15 00-59

Background

As noted in #491, multiple users raised the need for "off" switch:

  • No obvious one-click to "enable/disable" what IPFS Companion does.
    • Multiple users raised the need for "off" switch.
    • Removal of all redirects of IPFS paths and window.ipfs object during web development requires going over multiple checkboxes in Preferences or disabling the entire extension via Addon Manager (was the case when troubleshooting CORS bugs).
    • No clear indication when "power-user ipfs mode" (with local node, API and window.ipfs) is enabled, and when "vanilla public" (no API) gateway is used. This makes troubleshooting of various issues harder than necessary.

Another issue: a toggle between External (js-ipfs-api) and Embedded (js-ipfs) node types in the main UI:

  • Turns out users rarely change this setting (and sometimes instinctively assume it is a sort of an on/off switch!)
  • Our future integration with Brave will change the meaning of "Embedded" node, current UI does not translate to that well
  • Finally, it oversells the state of js-ipfs running is browser context:
    • It is still WIP experiment with known issues in browser context: High CPU usage of embedded node #450, and we should do a better job in communicating that to the user,
    • After bootstrap nodes and public gateways were separated, the UX of embedded node degraded severely.
      • It will improve after circuit-relaying with automatic relay discovery is shipped, but will take time. Until then we should adjust main UI and de-emphasize embedded node it in favor of using external one.

Changes

This PR delivers an initial part of #491:

  • Moves status fields back to the space-header, resulting in two clear sections instead of three vague ones
  • Replaces node type toggle with a global On/Off switch that enables/disables gateway redirect and all active API integrations.
    • Perfect for users who don't want to run js-ipfs all the time to save battery, want to disable redirect or temporarily block access to API window.ipfs
    • Things that don't require access to API will still work in OFF state:
      linkify, copy IPFS Path, custom protocol handlers
      (they just redirect to public gw)
  • Moves switch between External (js-ipfs-api) and Embedded (js-ipfs) to Preferences screen (screenshot)
  • Hides unused options when embedded node is active (Preferences screen)
  • Changes fonts to ones from ipfs-css

Open Questions

  • Does the updated browser action popup look good on Mac?
    (We had issues with pop-up in past, would be useful to review and confirm by a Mac user)

- 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 lidel added the UX label Jun 15, 2018
@lidel lidel added this to the v2.4.0 milestone Jun 15, 2018
@olizilla
Copy link
Member

@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?

@lidel
Copy link
Member Author

lidel commented Jun 15, 2018

@olizilla I think keeping and fading-out status items is indeed a better way, just added a commit that does just that:

1

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:

  • Perhaps we could reorder menu items so that it is less janky?

  • Lack of footer does not really bother me, but perhaps we could restore balance by moving "Share via IPFS to the very bottom and styling it differently (darker + icon) ?

This commit solves some regressions related to background page access
being denied in incognito mode.

More context:
#477
#243
@olizilla
Copy link
Member

Yep, that is better. How about including the menu options, I think it'll look calmer if the box doesn't resize.

@alanshaw
Copy link
Member

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?
  • 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?
  • 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
  • sanity double check - is disabling the add-on in the browser preferences effectively the same thing?
  • 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?

@olizilla
Copy link
Member

olizilla commented Jun 17, 2018 via email

@olizilla
Copy link
Member

olizilla commented Jun 17, 2018 via email

@lidel
Copy link
Member Author

lidel commented Jun 17, 2018

How about including the menu options, I think it'll look calmer if the box doesn't resize.

@olizilla It indeed may make things more calm, but may introduce some dead entries. (RED) ones will be inactive unless user is on /ipfs/ path. (PINK) ones may also get deactivated based on node type and its peering state, but that should be fine, I think.
As a compromise, would it be ok to always show everything, but hide section with RED ones in non-IPFS contexts?

screenshot_3

naming - I’m not sure if there’s a good answer but could users confuse on/off with actually starting/stopping the ipfs node?

@alanshaw Good question. Would something like "Active" / "Suspended" communicate the intent better?

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?

I agree we should de-emphasize the toggle. Moving it to the corner is a viable option.
Current look is just a first iteration to get it out there to the Beta channel.
When we add "node-type-selector" the on/off will not be that prominent.

@lidel
Copy link
Member Author

lidel commented Jun 18, 2018

@olizilla just pushed version that does not hide inactive menu items, but hides context-items in non-ipfs contexts:

Non-IPFS Context

peek 2018-06-18 12-34

IPFS Context

peek 2018-06-18 12-39

@lidel
Copy link
Member Author

lidel commented Jun 18, 2018

Do we want to move the toggle before this PR is merged?
If so, would it be better to move the switch to the bottom or the top corner?

Bottom

screenshot_39

Top Corner

screenshot_38


Personally I like top right corner better as it does not add any height to the UI, making it more compact.
Together with tooltip saying "Activate/Suspend gateway redirect and all active API integrations" it could be enough :)

@alanshaw
Copy link
Member

I like top right corner better

+1

@olizilla
Copy link
Member

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/

@lidel
Copy link
Member Author

lidel commented Jun 19, 2018

@olizilla I really want to avoid label, as it may bring too much focus to the toggle.
Personally, I would not stress over label too much: even if user does not intuit the meaning behind toggle in browser action, "Open Preferences" is the only active menu item, and the toggle is there as well, but with a clear label and color hint:

screenshot_9


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):

screenshot_40

screenshot_41

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:

screenshot_42

@olizilla
Copy link
Member

I like it! The last one with the power and config icons in the middle is my favourite.

@alanshaw
Copy link
Member

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)
@lidel
Copy link
Member Author

lidel commented Jun 20, 2018

Done!
Is there anything to tweak before this can be merged and released to Beta?

peek 2018-06-20 11-10

@alanshaw
Copy link
Member

🔥 HAWT

Just to check, does the cog open preferences? Do we need a menu item in additon to the cog?

@lidel
Copy link
Member Author

lidel commented Jun 20, 2018

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):

screenshot_11

I wonder if we should de-colourize icons when API is down (make them white/snow)

@olizilla
Copy link
Member

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 besnow white when the power is off.

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 👍 ✨

@lidel
Copy link
Member Author

lidel commented Jun 20, 2018

Thanks!

When in "power off' state, icons look more actionable now:

screenshot_12

As suggested, dropped duplicate "Open Preferences" from the menu to decrease clutter.
Other popular extensions (such as uBlock) use small icons to save screen space, so it should not be a problem:

screenshot_13
screenshot_15

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.

@lidel lidel merged commit d257ebc into master Jun 20, 2018
@lidel lidel deleted the feat/off-switch branch June 20, 2018 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants