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.

Sign up for GitHub

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

Jump to bottom

beastify needs a review #28

Closed
wbamberg opened this issue Jan 14, 2016 · 2 comments
Closed

beastify needs a review #28

wbamberg opened this issue Jan 14, 2016 · 2 comments

Comments

@wbamberg
Copy link

https://github.com/mdn/webextensions-examples/tree/master/beastify.
@bwinton
Copy link
Contributor

bwinton commented Jan 18, 2016

  • The README could use some content, describing what this add-on does, and why, and what it is trying to demonstrate.
  • In the content script it seems a little odd to add beastify as a listener before it's defined. (I know it works because functions get hoisted to the top, but changing the declaration to var beastify = function () {… which should be the same, will break it.)
  • I also kind of feel like the call to chrome.extension.getURL should happen in the popup code, because it feels less like a content thing to me, but I appreciate that this point is pretty arbitrary. 😉

wbamberg pushed a commit that referenced this issue Jan 19, 2016
Fix review comments: #28 (comment)
d552033 
* added README
* do name->URL conversion in the popup
* define functions before they are added as listeners
@wbamberg
Copy link
Author

Thanks @bwinton ! I've just committed: d552033 which should fix these.

wbamberg pushed a commit that referenced this issue Jan 20, 2016
Merge branch 'master' into chillout
6008ae6 
* master: (26 commits)
  Fix review comments: #31 (comment)
  Fix review comments: #30 (comment)
  Fix review comments: #29 (comment)
  Fix review comments: #28 (comment)
  added comments to one missing file
  added short comments for all JS functions
  ask that examples should contain the  key
  added strict_min_version to manifest.json
  ask that examples include the  key
  use the right size icons
  use the 'icons' key for all examples
  updates and typo fixes for CONTRIBUTING.md
  added draft CONTRIBUTING.md
  corrected a  comment
  added homepage_url to notify-link-clicks-i18n
  minor css tweaks on beastify popup
  use an absolute url for chrome.tabs.executeScript (which works on both Firefox and Chrome)
  increase popup width
  improve link detection in notify-link-clicks-i18n
  improve link detection in notify-link-clicks
  ...
wbamberg pushed a commit that referenced this issue Feb 19, 2016
Merge remote-tracking branch 'origin/master'
43961fd 
* origin/master: (34 commits)
  add _locales/ja
  Apply minor tweaks and fix typos in inpage-toolbar-ui doc and comments
  Minor tweaks on inpage-toolbar-ui stylesheet
  Add inpage-toolbar-ui add-on example
  Add 48px icon in 'icons' key
  Added README; stop using alarm name for tab ID
  Fix review comments: #31 (comment)
  Fix review comments: #30 (comment)
  Fix review comments: #29 (comment)
  Fix review comments: #28 (comment)
  add alarms permission, very minor update to a comment
  added comments to one missing file
  added short comments for all JS functions
  ask that examples should contain the  key
  added strict_min_version to manifest.json
  ask that examples include the  key
  use the right size icons
  use the 'icons' key for all examples
  updates and typo fixes for CONTRIBUTING.md
  added draft CONTRIBUTING.md
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bwinton @wbamberg