-
Notifications
You must be signed in to change notification settings - Fork 278
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
POC tota11y as Chrome extension/Firefox add-on #131
POC tota11y as Chrome extension/Firefox add-on #131
Conversation
Hey @nickytonline, Thanks for the PR! Mind signing our Contributor License Agreement? When you've done so, go ahead and comment Yours truly, |
e918cff
to
c691341
Compare
[clabot:check] |
CLA signature looks good 👍 |
20d3d7b
to
aa0760f
Compare
I completely forgot that you can use the same API and conventions for Firefox, so this will work as an extension in FireFox and Chrome. |
README.md
Outdated
|
||
then in Chrome, enter `chrome://extensions` in the address bar to load the Chrome extensions page. At the top left of the Chrome extensions page, click the *Load Unpacked* button. From the file browser, select the `./build` folder and press the *Select* button to load the tota11y Chrome extension unpacked. | ||
|
||
## Loading tota11y as an temporary add-on in Firefox |
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: "as an temporary" -> "as a temporary"
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.
Fixed.
@@ -6,6 +6,7 @@ let webpack = require("webpack"); | |||
let autoprefixer = require("autoprefixer"); | |||
|
|||
let options = require("./utils/options"); | |||
const CopyWebpackPlugin = require("copy-webpack-plugin"); |
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.
Great! Thanks for using const
.
webpack.config.babel.js
Outdated
new CopyWebpackPlugin([ | ||
{ | ||
from: "./extension-config", | ||
to: "./" |
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.
So this copies the config to the output directory? Would you mind adding a comment to that effect?
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.
It copies the the extension configuration and any extension assets to the build folder.
Thanks for this PR, @nickytonline! Great stuff. I had a couple of inline comments; nothing that you necessarily have to act on, but I'd love to hear your feedback. Looking forward to having tota11y as a browser extension! |
.gitignore
Outdated
@@ -1,4 +1,5 @@ | |||
node_modules/ | |||
tota11y/ | |||
build/ |
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.
Given our current build/release process, I'm not sure we can ignore the build
directory just yet.
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, I see in your master that the build folder is included in the repo. I'll revert this.
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.
Done
@somewhatabstract, I was on vacation. I'll check out your feedback this week and update the PR. |
e21995a
to
6b88e21
Compare
webpack.config.babel.js
Outdated
@@ -34,6 +35,13 @@ const plugins = [ | |||
new webpack.ProvidePlugin({ | |||
[options.jsxPragma]: path.join(__dirname, "utils", "element"), | |||
}), | |||
// Copies the extension configuration and other extension assets to the build folder. |
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.
A little explanation of what this does.
87a5d67
to
74c1492
Compare
.eslintrc
Outdated
@@ -22,6 +22,7 @@ | |||
}, | |||
"globals": { | |||
"axs": true, | |||
"$": true | |||
"$": true, | |||
"chrome": false |
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 global is added for the Chrome extension scripts.
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 can actually move this global to a .eslintrc file in the extension configuration folder.
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.
Done.
{ | ||
"matches": [ | ||
"https://*/*", | ||
"http://*/*" |
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.
Even though it's 2019, some sites still use http, so it needs to be supported. YOLO
@@ -0,0 +1,39 @@ | |||
<!DOCTYPE html> |
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 went with whatever for the markup and tota11y icon. If you would like this tweaked, let me know.
extension-config/popup.js
Outdated
|
||
function activateTota11y() { | ||
console.log('activating tota11y...'); | ||
chrome.tabs.executeScript(null, { file: "tota11y.js" }); |
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.
Only load the script when the user explicitly clicks the checkbox in the extension popup.
So just a little summary of the approach I took for this. I know the actual script is used as a bookmarklet, so I did not want to interfere with any of that. The added webpack configuration just copies over the extension configuration to the build folder. The way the extension works is it only loads the tota11y script once the user checks the checkbox in the extension popup. The reason for doing this is we don't want the script loading all the time, only when a developer wants to explicitly use it. See https://github.com/Khan/tota11y/pull/131/files#diff-761b689f243e9bf60b5a7effc677551fR13 This technically should work in any Chromium based browser that uses the Chrome Web store, so this means when the new Edge is released, it should work there as well unless they go with their own extension store which is possible. It also works in the Brave Browser, my current browser of choice. Update May 2019 I figured it would work, but just confirmed that tota11y works in Edge on MacOS. I'll assume it's good to go on Windows as well. |
I purposely haven't checked in the assets for the extension that get copied to the If you'd like them in the build folder, let me know and I'll commit them. As is, this PR allows developers to use the extension unpacked as mentioned in the readme. To deploy to the Chrome and FireFox extension stores, IDs will need to be acquired and you'll need to follow the steps for deploying to each of those stores respectively. The extension already has an icon which I took from your existing logo. Feel free to tweak it if necessary. Here's some helpful links if no one on the team has ever deployed an extension: |
dbcf319
to
5370393
Compare
extension-config/popup.js
Outdated
@@ -11,18 +11,19 @@ function getActiveTab() { | |||
function activateTota11y() { | |||
console.log('activating tota11y...'); | |||
chrome.tabs.executeScript(null, { file: "tota11y.js" }); | |||
console.log('t0ta11y has been activated.'); | |||
console.log('tota11y has been activated.'); |
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.
Fixed a typo.
I haven't forgotten this. I will try to make time this week to give feedback. Thank you for your patience. |
f2254eb
to
47dce9d
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.
This is looking pretty good to me. I have some outstanding questions that hopefully you can brainstorm with me:
- Should we publish this extension to the Chrome Store when we publish the npm package?
- If so, how should we coordinate that? Updated publish instructions? Scripts?
- How do we keep the extension version and npm package version in sync?
- Should the extension be part of the npm package? (I'm thinking not, so we should make sure that it doesn't get included when the package gets put together)
@kevinbarabash Do you have any thoughts about this?
README.md
Outdated
Build the project | ||
|
||
``` | ||
npm run build |
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.
build or build:prod?
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.
Ahh, I see you made some changes about a month ago. I'd go with npm run
build:prod`. I'll update the documentation.
README.md
Outdated
Build the project | ||
|
||
``` | ||
npm run build |
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.
build or build:prod?
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.
Same as above.
@@ -0,0 +1,40 @@ | |||
function toggleTota11y(payload, sendResponse) { |
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.
Could you add a comment at the top of the file explaining what the file contents are for?
"manifest_version": 2, | ||
"name": "tota11y", | ||
"description": "An accessibility visualization toolkit", | ||
"version": "1.0", |
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.
Should we match the version to our npm package version?
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.
If so, should we add some scripting to ensure that versions stay aligned?
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 would bump the version, currently 0.1.6 on npm and 0.2.0 in the package.json to 0.3.0 since the changes are minor in the sense there are no breaking changes, just additional functionality via the browser extension. I ran npm version minor
to do that. I'd suggest just running npm version x
based on what you want to do in terms of npm versioning before publishing. I'm happy to add some more scripting, but I'm not sure much more is needed between npm version
and the pre-publish-checks.js. Thoughts?
extension-config/popup.html
Outdated
<img src="assets/icon-128.png" /> | ||
<a href="https://khan.github.io/tota11y" target="_blank" rel="noopener">tota11y</a> | ||
<p>An accessibility visualization toolkit</p> | ||
<label for="tota11y-toggler">Show tota11y<input id="tota11y-toggler" type="checkbox" /></label> |
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.
If this is already showing, should it say "Hide tota11y"? Or, just change it to Toggle tota11y
?
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.
Initially tota11y is not showing. I'll update the text to Toggle tota11y
.
@@ -0,0 +1,63 @@ | |||
function getActiveTab() { |
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 you add a comment to the top of the file explaining the purpose of this code?
to: "./", | ||
ignore: [ ".eslintrc" ] | ||
} | ||
]) |
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, is the extension different between prod and dev builds? Do we need to do anything special between dev and prod?
… can still run as a bookmarklet on it's own without any extension specific stuff attached to it.
47dce9d
to
258dd40
Compare
Closing this as it does work, but needs some love to get it in the extension stores. If this is something you still want to pursue, feel free to reopen. |
Hi, I have installed the Chrome extension of tota11y and when I try to check my website it's not working, Can some one please help me on this. |
Hi @dineshdesigner08. This was never an official Chrome extension. It was something that I thought would be interesting to do and I also created this PR to show interest in Khan Academy when I applied there a long time ago. With that said there's no support for this, but if you install the extension unpacked, it should work. Just follow the instructions in the README in the changed files in the PR. |
Thanks for the update @nickytonline, will follow the README file, Very much Appreciate |
@dineshdesigner08, out of curiosity, how did you stumble upon my PR? |
Came across #62 after I created this. Thought it'd be fun to make it an extension and apparently others do too according to #62.