-
Notifications
You must be signed in to change notification settings - Fork 325
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
Add Protocol Indicator to Location Bar in Firefox #418
Conversation
Right now left-click on the icon does nothing, and right click displays context menu. Related feedback from IRC:
|
- 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)
Added Menu Under
|
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 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).
@@ -163,18 +163,23 @@ module.exports = (state, emitter) => { | |||
|
|||
state.isIpfsContext = !!ipfsContext | |||
state.currentTabUrl = status.currentTab ? status.currentTab.url : null |
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.
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)
<html> | ||
<head> | ||
<meta charset="utf-8"> | ||
<meta name="viewport" content="width=device-width" /> |
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.
can remove the trailing slash /
<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;"> |
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.
to avoid potential specificity issues later, perhaps move these styles to a <style></style>
block at the bottom of the <head>
?
add-on/src/popup/page-action/page.js
Outdated
const header = require('./header') | ||
const contextActions = require('../browser-action/context-actions') | ||
|
||
// Render the page action page: |
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.
nit: to reduce ambiguity when reading this: page action
-> page-action
(or pageAction
)?
add-on/src/popup/page-action/page.js
Outdated
contextActionsProps.isIpfsContext = true | ||
|
||
return html` | ||
<div class="helvetica"> |
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.
minor, but just curious: is the helvetica
class preferred over the sans-serif
("system font") class?
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.
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?
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 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.
if (!isIpfsContext) return null | ||
return html` | ||
<div class="ph2 pv1 br2 br--top bg-light-gray"> | ||
<h2 class="f6 mt2 mb2 tl fw1"> |
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.
you could collapse mt2 mb2
to mv2
, right?
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.
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.
app.route('*', pageActionPage) | ||
|
||
// Start the application and render it to the given querySelector | ||
app.mount('#root') |
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 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?
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.
#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
add-on/src/popup/page-action/page.js
Outdated
const onUnPin = () => emit('unPin') | ||
const contextActionsProps = Object.assign({ onCopyIpfsAddr, onCopyPublicGwAddr, onPin, onUnPin }, state) | ||
|
||
// instant init: page action is shown only in ipfsContext |
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.
nit: Capitalise comment for consistency?
expect(ipfsPathValidator.validIpfsOrIpnsUrl(url)).to.equal(false) | ||
}) | ||
}) | ||
describe('publicIpfsOrIpnsResource', function () { |
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 tests here, woo! 👍
|
||
// https://github.com/ipfs/ipfs-companion/issues/303 | ||
describe('IPFS Path Sanitization', function () { | ||
describe('ipfs-path.js', function () { |
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 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?
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 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
to answer your question in #418 (comment):
I'd recommend Option A I think. as long as users aren't hopping between multiple browsers and get used to Firefox supporting 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. |
@cvan Thank you for feedback! I think most of things got addressed in e8926a0. I suspect Windows support could be broken due to #416, namely use of Regarding Option A: let's go with it for now. There is no maintenance cost (we already share code between I feel current Future Work
|
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:
As noted in #398, this is Firefox-only for now.
Demo (click to zoom)