/landmarks forked from davidtodd/landmarks

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

Consolidate code and conditional build #191

Merged
merged 17 commits into from Aug 4, 2018

Conversation

Reviewers

@MarkWithall MarkWithall

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@matatk @MarkWithall
@matatk
Owner

matatk commented Aug 4, 2018

This massively re-organises the code around ES6 modules, to make it much simpler when built, so that:

  • Things like the injector are much simpler.
  • It’s easier to add/remove code.
  • It’s easy to do cross-browser behavioural differences and not include code/data that needn’t be.

Fixes #189

matatk added some commits Jul 30, 2018

@matatk
Use rollup 
* This just-about works on Firefox; need to do build environment
  variables next, and a lot of clean-up. It's a mess for any browser
  other than Firefox. Also the code that represents the entry points
  should be named appropriately (e.g. '_content.js'?) and all of the
  filenames need looking at. Finally, the names of exports should be
  looked at---should they be hardcoded, or default?
* Updated tests to use rollup-generated test code (much simpler; very
  neat and easy :-)).
* Had to disable linting the build/ dir afterwards (due to semicolons
  etc.); would like to re-enable that.
ce57fe4
@matatk
Tidy up script ignores 
* This ignores and removes certain scripts. In future it would be neater
  for them to be generated and go directly to their build directories,
  rather than stop over in src/static/.
* Also ensure rollup is run before a quick or a normal build.
ccbbe48
@matatk
Rollup in the build script; Make it sync
3417085
@matatk
Make it work on Firefox (62+) 
* Set open_at_install on the sidebar to false, so that the starting
  state is always known (need to use this approach until isOpen() is fixed
  to propogate user input blessedness for event handling).
* Set minimum required Firefox version to 62 as required (partly
  addresses #190).
* Use Firefox Developer Edition for testing (due to the requirement for
  Firefox 62).
* Also open the Browser Console by default when using web-ext.
8829e9c
@matatk
Rename src/js/ to src/code/
25ebdfc
@matatk
Use rollup-plugin-terser 
* Update deps.
* Update build script to use the plugin.
* Test the plugin in the code. This appears to've uncovered a bug with
  terser: terser-js/terser#92
b84fa82
@matatk
Use best options for terser 
* Thanks for the help on terser-js/terser#92 :-)
5fd68b0
@matatk
Cross-browser running again 
* Remove loadsa script inclusions from manifests.
* Create cross-browser compatibility script and import.
* Create cross-browser specialPages script and import.
* Remove now-defo-unused code from build script.
* Increase terser compress passes to two to avoid dangling strings.
7116ed0
@matatk
Rename code files for clarity
5b5db22
@matatk
Factor out Logger; Re-name exports; Tidy imports 
* Factor out Logger.
* Give all default exports their names back.
* Remove redundant ".js" suffixes from imports.
* Remove options-change logging (debugging info).
* Rename contrast.js to contrastChecker.js to reflect name.
8b7c4e0
@matatk
Fix options on Chrome; Remove script links
f5e2791
@matatk
Fix content script injector 
* Reinstate the content script injector. This always has to be imported,
  but can return null on Firefox.
* Set terser's `conditionals` compressor option so that simple tests for
  one browser can be filtered out.
6ecad2a
@matatk
Only do sidebar stuff on sidebar-supporting browsers
0310a52
@matatk
Use rollup-plugin-esformatter 
Alas the output after rollup and terser was a bit wonky. Experimenting
with two spaces for now, as whilst I doubt it will be sufficiently
readable, it might take up less space in browser inspector windows, so
may be good; will see...
1e6b83d
@matatk
Browser-appropriate defaultSettings 
* Remove interface defaultSettings from Chrome and Edge.
* Remove debugging info.
59e7dd2
@matatk
Trying to figure out Opera 
* Remvoe some but not all debugging things; trying to figure out why
  Opera is not showing the browserAction toolbar button sometimes.
Loading status checks…
0a180ee
@matatk
Note code consolidation changes in README
Loading status checks…
35d92db

@matatk matatk merged commit 22fef45 into master Aug 4, 2018
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@matatk matatk deleted the consolidate-code-and-conditional-build branch Aug 4, 2018

README.md
@@ -210,6 +210,7 @@ Changes
- 2.4.0 - in development
- Show the current keyboard commands on the splash page and allow the user to update them on Chrome and Opera. \[[\#187](https://github.com/matatk/landmarks/pull/187)\]
- Offer an optional sidebar instead of the pop-up on Firefox and Opera. \[[\#188](https://github.com/matatk/landmarks/pull/188)\]
- Massive re-organisation of the code to make it easier to manage and accommodate and take advantage of cross-browser differences. \[[\#191](https://github.com/matatk/landmarks/pull/191)\]

This comment has been minimized.

Sign in to view
Copy link
@MarkWithall

MarkWithall Aug 6, 2018

This seems to have a tab, whereas the rest has spaces.

This comment has been minimized.

Sign in to view
Copy link
@matatk

matatk Aug 6, 2018

Owner

Oops; good catch; thanks :-). I usually remember to replace them. Should probably reformat the whole list, but as it gets longer, the task gets less desirable... :-)

@matatk matatk referenced this pull request Aug 8, 2018

Closed

Fix spacing in README #193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment