search

mozilla / lightbeam-we

Web Extension version of the Firefox Lightbeam add-on
https://addons.mozilla.org/en-GB/firefox/addon/lightbeam/
Mozilla Public License 2.0
181 stars 61 forks source link

Restructure lightbeam.js similar to other scripts #160

Closed biancadanforth closed 7 years ago

biancadanforth commented 7 years ago

@jonathanKingston Good idea to make this a separate PR to the dynamic variables...

Basically I just made the lightbeam.js script of the same form as capture.js, store.js, etc., where lightbeam is a global object with methods. I have not modified any methods, just moved things around and renamed the anonymous function that passed an update callback to storeChild as redraw.

jonathanKingston commented 7 years ago

Can you outline the changes here this is a pretty noisy diff. Could you remove the WIP and fixes as I think this should just be a cleanup bug instead and not mixed with a much bigger patch? Refractors should be treated in isolation and with as much rigour as anything else. Feel free to raise a bug and mark the fixes as that bug id.

On Mon, 21 Aug 2017, 22:45 Bianca Danforth notifications@github.com wrote:

[WIP] Haven't added the dynamic variables yet - just cleaned up the script. Ultimately, this is part of Issue #65 https://github.com/mozilla/lightbeam-we/issues/65.

You can view, comment on, or merge this pull request online at:

https://github.com/mozilla/lightbeam-we/pull/160 Commit Summary

  • Restructure lightbeam.js similar to other scripts

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUsLMrsNFr8fUMjDY3b20IYJ1gk-bBFks5safpwgaJpZM4O93UY .

jonathanKingston commented 7 years ago

This looks ok, can you change lightbeam in init() to use this to be consistent.

On Mon, 21 Aug 2017, 23:43 Jonathan Kingston jonathan@jooped.co.uk wrote:

Can you outline the changes here this is a pretty noisy diff. Could you remove the WIP and fixes as I think this should just be a cleanup bug instead and not mixed with a much bigger patch? Refractors should be treated in isolation and with as much rigour as anything else. Feel free to raise a bug and mark the fixes as that bug id.

On Mon, 21 Aug 2017, 22:45 Bianca Danforth notifications@github.com wrote:

[WIP] Haven't added the dynamic variables yet - just cleaned up the script. Ultimately, this is part of Issue #65 https://github.com/mozilla/lightbeam-we/issues/65.

You can view, comment on, or merge this pull request online at:

https://github.com/mozilla/lightbeam-we/pull/160 Commit Summary

  • Restructure lightbeam.js similar to other scripts

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUsLMrsNFr8fUMjDY3b20IYJ1gk-bBFks5safpwgaJpZM4O93UY .

jonathanKingston commented 7 years ago

Which probably will require the onload to be an anon arrow function that calls init().

On Tue, 22 Aug 2017, 02:39 Jonathan Kingston jonathan@jooped.co.uk wrote:

This looks ok, can you change lightbeam in init() to use this to be consistent.

On Mon, 21 Aug 2017, 23:43 Jonathan Kingston jonathan@jooped.co.uk wrote:

Can you outline the changes here this is a pretty noisy diff. Could you remove the WIP and fixes as I think this should just be a cleanup bug instead and not mixed with a much bigger patch? Refractors should be treated in isolation and with as much rigour as anything else. Feel free to raise a bug and mark the fixes as that bug id.

On Mon, 21 Aug 2017, 22:45 Bianca Danforth notifications@github.com wrote:

[WIP] Haven't added the dynamic variables yet - just cleaned up the script. Ultimately, this is part of Issue #65 https://github.com/mozilla/lightbeam-we/issues/65.

You can view, comment on, or merge this pull request online at:

https://github.com/mozilla/lightbeam-we/pull/160 Commit Summary

  • Restructure lightbeam.js similar to other scripts

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUsLMrsNFr8fUMjDY3b20IYJ1gk-bBFks5safpwgaJpZM4O93UY .

biancadanforth commented 7 years ago

Oh I forgot to mention 'this' binding points to window there after the implicit reassignment on the previous line, so directly using 'this' doesn't work... Any suggestions?

On Aug 21, 2017 6:39 PM, "Jonathan Kingston" notifications@github.com wrote:

This looks ok, can you change lightbeam in init() to use this to be consistent.

On Mon, 21 Aug 2017, 23:43 Jonathan Kingston jonathan@jooped.co.uk wrote:

Can you outline the changes here this is a pretty noisy diff. Could you remove the WIP and fixes as I think this should just be a cleanup bug instead and not mixed with a much bigger patch? Refractors should be treated in isolation and with as much rigour as anything else. Feel free to raise a bug and mark the fixes as that bug id.

On Mon, 21 Aug 2017, 22:45 Bianca Danforth notifications@github.com wrote:

[WIP] Haven't added the dynamic variables yet - just cleaned up the script. Ultimately, this is part of Issue #65 https://github.com/mozilla/lightbeam-we/issues/65.

You can view, comment on, or merge this pull request online at:

https://github.com/mozilla/lightbeam-we/pull/160 Commit Summary

  • Restructure lightbeam.js similar to other scripts

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160, or mute the thread https://github.com/notifications/unsubscribe-auth/ AAUsLMrsNFr8fUMjDY3b20IYJ1gk-bBFks5safpwgaJpZM4O93UY .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160#issuecomment-323895452, or mute the thread https://github.com/notifications/unsubscribe-auth/AQoS_B_wBrpltZbwjJW0OrSrOejSzD9sks5sajFcgaJpZM4O93UY .

jonathanKingston commented 7 years ago

Yeah my follow up email got lost perhaps:

window.onload = () => { Lightbeam.init(); };

On Tue, 22 Aug 2017, 02:53 Bianca Danforth notifications@github.com wrote:

Oh I forgot to mention 'this' binding points to window there after the implicit reassignment on the previous line, so directly using 'this' doesn't work... Any suggestions?

On Aug 21, 2017 6:39 PM, "Jonathan Kingston" notifications@github.com wrote:

This looks ok, can you change lightbeam in init() to use this to be consistent.

On Mon, 21 Aug 2017, 23:43 Jonathan Kingston jonathan@jooped.co.uk wrote:

Can you outline the changes here this is a pretty noisy diff. Could you remove the WIP and fixes as I think this should just be a cleanup bug instead and not mixed with a much bigger patch? Refractors should be treated in isolation and with as much rigour as anything else. Feel free to raise a bug and mark the fixes as that bug id.

On Mon, 21 Aug 2017, 22:45 Bianca Danforth notifications@github.com wrote:

[WIP] Haven't added the dynamic variables yet - just cleaned up the script. Ultimately, this is part of Issue #65 https://github.com/mozilla/lightbeam-we/issues/65.

You can view, comment on, or merge this pull request online at:

https://github.com/mozilla/lightbeam-we/pull/160 Commit Summary

  • Restructure lightbeam.js similar to other scripts

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160, or mute the thread https://github.com/notifications/unsubscribe-auth/ AAUsLMrsNFr8fUMjDY3b20IYJ1gk-bBFks5safpwgaJpZM4O93UY .

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <https://github.com/mozilla/lightbeam-we/pull/160#issuecomment-323895452 , or mute the thread < https://github.com/notifications/unsubscribe-auth/AQoS_B_wBrpltZbwjJW0OrSrOejSzD9sks5sajFcgaJpZM4O93UY

.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/mozilla/lightbeam-we/pull/160#issuecomment-323897255, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUsLIg_S8ZVyI4r7kaQL_XTgktuYbeLks5sajSHgaJpZM4O93UY .

biancadanforth commented 7 years ago

Okay @jonathanKingston , I updated the onload event to use an arrow function in the callback. Ready for you!

jonathanKingston commented 7 years ago

Useful tip @biancadanforth checking this branch out and doing git show <commit-id> -w makes the commit super readable. I dunno why github defaults to showing the whitespace changes certainly for JS files at least.