Add Protocol Indicator to Location Bar in Firefox #418

lidel · 2018-03-12T12:56:20Z

This PR closes #398 and shows pageAction in location bar if current page is IPFS resource.
Different color and tooltip are shown when resource was loaded via local gateway:

grey icon – valid IPFS resource, but loaded from a public gateway (via HTTP)
aqua icon – IPFS resource loaded via user's custom gateway (actual IPFS was used)

As noted in #398, this is Firefox-only for now.

Demo (click to zoom)

closes #398

lidel · 2018-03-12T14:02:10Z

Right now left-click on the icon does nothing, and right click displays context menu.
Should left-click "Copy the Canonical NURI" ?
Or open a simple menu with 3 page-context-aware options (we have them in browserAction):

Related feedback from IRC:

135724           @lidel │ Protocol Indicator in Location Bar in Firefox: https://github.com/ipfs-shipyard/ipfs-companion/pull/418
135907        alanshaw_ │ That is awesome!
135938                  │ I'm not sure I understand the grey cube - why not just always coloured?
140504           @lidel │ grey -- loaded from public gateway (HTTP) 
140520                  │ aqua - loaded from custom gateway (IPFS)
140531        alanshaw_ │ yeah but why grey for that? Looks disabled
140658           @lidel │ Honest answer would be: because we have only 2 icons :) Longer one: because if public gateway was used, then IPFS was disabled, old HTTP
                        │ was used.
140752           @lidel │ I am perfectly fine with changing icons/tooltips, this is just an initial stab at it :)
150034        alanshaw_ │ thats cool, I guess a "IPFS over HTTP" icon might be needed...
151203           @lidel │ created Design: IPFS Protocol Indicator Icon https://github.com/ipfs-shipyard/pm-ipfs-gui/issues/30
151439        kyledrake │ That indicator is nice.
152122           @lidel │ I wonder if we should give it "a default action": https://github.com/ipfs-shipyard/ipfs-companion/pull/418#issuecomment-372319767
153031        alanshaw_ │ pin?
153255                  │ Those icons seem to indicate that there is something that can be done with this page
153334                  │ I have  https://usercontent.irccloud-cdn.com/file/eY6on4Fm/Screen%20Shot%202018-03-12%20at%2014.33.20.png
153427           @lidel │ yeah, pageAction can have onClick event or it can display a popup just like browserAction does
153432        alanshaw_ │ What if this icon is a shortcut for pinning/unpinning?
153607           @lidel │ not sure if it should be that easy to pin, we would have to provide some feedback to user, eg. indicate pin/unpin somehow in icon
153616                  │ what if we just display menu 
153652                  │ and there would be 3 options (same ones as in browserAction: copy NURI, copy URI and pin/unpin) 
154438        alanshaw_ │ I think it would be awesome if the icon could convey pinned status for the resource also
154547        kyledrake │ Those options sound good.  I would make the pinning the first option in that case. I'd love to see more people pinning content.
154633                  │ It would be cool if it could show how many people it sees with the resource on the network, but I suppose that's too much to ask here.
170713           @lidel │ Yeah, even if there is an API for that, there will be an overhead for getting such stats (we have simple stats on the upload screen and
                        │ it lags sometimes). Workaround would be  to display 3 options in menu and asynchronously load more stats in background.

- implements feedback from #418 (comment) - re-use actions and code from browserAction - actual gateway info at the time of loading is displayed (this means user can toggle between public/custom gateway and it will not change already existing indicator)

lidel · 2018-03-14T01:10:36Z

Added Menu Under `pageAction`

code re-use from browserAction
tried to make it as snappy as possible
- expensive things that require API call are fetched and updated later (for now it is only Pin status)

Open Question

Option A should we keep these menu items in both pageAction and browserAction?
Option B we could detect that browser supports pageAciton and hide them from browserAction

Demo

cvan

this looks really slick! nice work on getting this for Firefox (even if it meant special casing it 👍).

I wanted to provide this feedback, but I am not able to clone and test this. so take my comments for what they are - just comments, not actual feedback on the feature addition you've made.

on master, I'm having some issues building the project on Windows (which I'll file separate issues for).

cvan · 2018-03-14T07:41:38Z

add-on/src/popup/browser-action/store.js

@@ -163,18 +163,23 @@ module.exports = (state, emitter) => {

    state.isIpfsContext = !!ipfsContext
    state.currentTabUrl = status.currentTab ? status.currentTab.url : null


what do you think about having a single state.currentTab property instead of two separate ones?

state.currentTab = Object.assign({url: null, id: null}, state.currentTab)

cvan · 2018-03-14T07:43:46Z

add-on/src/popup/page-action/index.html

+<html>
+  <head>
+    <meta charset="utf-8">
+    <meta name="viewport" content="width=device-width" />


can remove the trailing slash /

cvan · 2018-03-14T07:44:53Z

add-on/src/popup/page-action/index.html

+    <link rel="stylesheet" href="../browser-action/browser-action.css">
+    <script src="../../ipfs-companion-common.js"></script>
+  </head>
+  <body style="width: 320px; overflow: hidden; background: white;">


to avoid potential specificity issues later, perhaps move these styles to a <style></style> block at the bottom of the <head>?

cvan · 2018-03-14T07:45:59Z

add-on/src/popup/page-action/page.js

+const header = require('./header')
+const contextActions = require('../browser-action/context-actions')
+
+// Render the page action page:


nit: to reduce ambiguity when reading this: page action -> page-action (or pageAction)?

cvan · 2018-03-14T07:49:57Z

add-on/src/popup/page-action/page.js

+  contextActionsProps.isIpfsContext = true
+
+  return html`
+    <div class="helvetica">


minor, but just curious: is the helvetica class preferred over the sans-serif ("system font") class?

also, it looks like ipfs-css has a custom, preferred font-family stack that overrides sans-serif. I assume sans-serif is safe/preferred to use here then?

Yeah, we are in the process of switching toipfs-css, but to keep PR small I just copied already existing style conventions from Browser Action.
Global style-related changes will go in separate PR at some point in future.

cvan · 2018-03-14T08:53:54Z

add-on/src/popup/page-action/header.js

+  if (!isIpfsContext) return null
+  return html`
+    <div class="ph2 pv1 br2 br--top bg-light-gray">
+      <h2 class="f6 mt2 mb2 tl fw1">


you could collapse mt2 mb2 to mv2, right?

while we're iterating on UI, it's just as useful to have the individual atoms. All feedback is welcome, but it'd help us to focus on issues that might cause a problem.

cvan · 2018-03-14T09:02:37Z

add-on/src/popup/page-action/index.js

+app.route('*', pageActionPage)
+
+// Start the application and render it to the given querySelector
+app.mount('#root')


I know WebExtensions are not the easiest thing to test, even with manual human testing. I assume the interactions, including this pageAction addition, will be handled by the smoke tests in #276, IIUC?

#276 is mostly for "checking current state of things" (Edge-related issue was spawned from it). Proper one is #145. But yes, for now we just need to eyeball things that can't be tested via mocha.

Some background:

In old SDK (legacy-sdk branch) we had very nice suite of tests that run in actual browser and utilized asserts against browser UI (example)

There is no such test library for WebExtensions APIs, but I described the need in mozilla/web-ext#5

cvan · 2018-03-14T09:03:07Z

add-on/src/popup/page-action/page.js

+  const onUnPin = () => emit('unPin')
+  const contextActionsProps = Object.assign({ onCopyIpfsAddr, onCopyPublicGwAddr, onPin, onUnPin }, state)
+
+  // instant init: page action is shown only in ipfsContext


nit: Capitalise comment for consistency?

cvan · 2018-03-14T09:03:40Z

test/functional/lib/ipfs-path.test.js

+      expect(ipfsPathValidator.validIpfsOrIpnsUrl(url)).to.equal(false)
+    })
+  })
+  describe('publicIpfsOrIpnsResource', function () {


nice tests here, woo! 👍

cvan · 2018-03-14T09:05:10Z

test/functional/lib/ipfs-path.test.js


 // https://github.com/ipfs/ipfs-companion/issues/303
-describe('IPFS Path Sanitization', function () {
+describe('ipfs-path.js', function () {


I assume no reason other than legacy that fat-arrow functions aren't used everywhere here? or is it because of mocha patching or just a pattern maintained?

I think it is just an unwritten convention so that we don't need to think where arrow function will be a problem for mocha: https://mochajs.org/#arrow-functions

cvan · 2018-03-14T09:09:57Z

to answer your question in #418 (comment):

Option A should we keep these menu items in both pageAction and browserAction?
Option B we could detect that browser supports pageAciton and hide them from browserAction

I'd recommend Option A I think. as long as users aren't hopping between multiple browsers and get used to Firefox supporting pageAction and another browser not... it's probably not a major issue.

are there any particular use cases you think could cause confusion? or just more of a code-maintainability concern?

P.S. nice work on adding the GIFs. they help a ton for everyone looking at this PR now (or later) to see what the change looked like at the time.

#418 (review)

lidel · 2018-03-14T11:47:57Z

@cvan Thank you for feedback! I think most of things got addressed in e8926a0.
For everything else feel free to fill separate issues or PRs.

I suspect Windows support could be broken due to #416, namely use of > here. Anyway, please fill a separate issue for it. We want people to be able to build it everywhere, so perhaps a Jenkins job that smoke-tests build on Windows is a good idea.

Regarding Option A: let's go with it for now. There is no maintenance cost (we already share code between pageAction and browserAction), my only worry is UX, eg. that page-specific items in browserAction are not visually different than regular ones. But this is how it was for a long time, so we can address it in separate PR.

I feel current pageAction UI is good enough for now: merging this in preparation for beta release compatible with Firefox 59.

Future Work

Design: IPFS Protocol Indicator Icon Design: IPFS Protocol Indicator Icon ipfs-gui#30
Improve support for dark/light themes in Firefox

Add Protocol Indicator in Location Bar

a42dc4e

closes #398

lidel added the UX label Mar 12, 2018

lidel added this to the 2018-Q1 milestone Mar 12, 2018

lidel self-assigned this Mar 12, 2018

lidel mentioned this pull request Mar 12, 2018

IPFS Protocol Indicator in Location Bar #398

Closed

lidel mentioned this pull request Mar 12, 2018

Design: IPFS Protocol Indicator Icon ipfs/ipfs-gui#30

Closed

cvan reviewed Mar 14, 2018

View reviewed changes

Apply feedback from discussion in PR #418

e8926a0

#418 (review)

lidel merged commit 0700884 into master Mar 14, 2018

lidel deleted the feat/protocol-indicator branch March 14, 2018 12:00

lidel mentioned this pull request Apr 13, 2018

Ideas for IPFS UX ipfs/in-web-browsers#53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Protocol Indicator to Location Bar in Firefox #418

Add Protocol Indicator to Location Bar in Firefox #418

lidel commented Mar 12, 2018 •

edited

Loading

lidel commented Mar 12, 2018 •

edited

Loading

lidel commented Mar 14, 2018 •

edited

Loading

cvan left a comment

cvan Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

lidel Mar 14, 2018

cvan Mar 14, 2018

olizilla Mar 15, 2018

cvan Mar 14, 2018

lidel Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

cvan Mar 14, 2018

lidel Mar 14, 2018

cvan commented Mar 14, 2018

lidel commented Mar 14, 2018

		@@ -163,18 +163,23 @@ module.exports = (state, emitter) => {

		state.isIpfsContext = !!ipfsContext
		state.currentTabUrl = status.currentTab ? status.currentTab.url : null

Add Protocol Indicator to Location Bar in Firefox #418

Add Protocol Indicator to Location Bar in Firefox #418

Conversation

lidel commented Mar 12, 2018 • edited Loading

Demo (click to zoom)

lidel commented Mar 12, 2018 • edited Loading

lidel commented Mar 14, 2018 • edited Loading

Added Menu Under pageAction

Open Question

Demo

cvan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvan commented Mar 14, 2018

lidel commented Mar 14, 2018

Future Work

lidel commented Mar 12, 2018 •

edited

Loading

lidel commented Mar 12, 2018 •

edited

Loading

lidel commented Mar 14, 2018 •

edited

Loading

Added Menu Under `pageAction`