Es6 #163

simobasso · 2017-01-02T14:19:36Z

move whole package to es6

codecov-io · 2017-01-02T14:24:49Z

Current coverage is 69.46% (diff: 69.46%)

No coverage report found for master at eda56e0.

Powered by Codecov. Last update eda56e0...ecdd2b2

dennybiasiolli · 2017-01-02T14:25:49Z

webpack.config.js

+          presets: [
+            "latest",
+          ],
+          "plugins": [ "istanbul" ]


please don't use double quotes in property name

dennybiasiolli · 2017-01-02T14:27:15Z

webpack.config.js

+    },
+    emitError: false,
+    failOnError: false,
+    "rules": {


please remove this rule

dennybiasiolli · 2017-01-02T14:27:57Z

webpack.config.js

+      ecmaVersion: 6,
+      sourceType: "module"
+    },
+    emitError: false,


please set true on emitError and failOnError

dennybiasiolli · 2017-01-02T14:28:16Z

webpack.config.js

+  },
+  jscs: {
+    validateIndentation: 2,
+    emitErrors: false,


please set true on emitError and failOnHint

dennybiasiolli · 2017-01-02T14:29:37Z

src/angular-pdf.directive.js

+      let scale = attrs.scale > 0 ? attrs.scale : 1;
+      let canvasid = attrs.canvasid || 'pdf-canvas';
+      let canvas = $document[0].getElementById(canvasid);
+      var creds = attrs.usecredentials;


can we use let also for creds, ctx and windowEl?

dennybiasiolli · 2017-01-04T14:08:10Z

package.json

  },
  "keywords": [
    "pdf",
    "angular",
    "directive"
  ],
-  "license": "MIT",
+  "dependencies": {


Please use peerDependencies instead of dependencies

node <= 0.xx doesn't support es6 syntax ref: http://node.green/

In order to use es6 in the package we need a module bundler (webpack) that transpile our code using babel. To do that we did: - added bumped to manage releases - updated travis test - updated package.json and bower.json in order to work with webpack - added a CHANGELOG page - updated index.html - added many fix on module - added eslint and remove jsonhint - removed bower dependency - updated documentation - added automatic gh-pages deploy - added angularjs and pdfjs as peerDependencies aside note of duplicated dependencies in package.json: angularjs and pdfjs are in both list because we need to alert user if they lack AND for developing/testig purposes ref: sayanee#90

Generate code coverage using Istanbul

dennybiasiolli · 2017-01-09T18:23:43Z

@simobasso can we also have an angular-pdf.js file in dist folder after building with webpack, like in the current master?

rhyskoedijk · 2017-04-08T23:59:11Z

Could I make a small suggestion since you are already writing this.
Consider exposing the PDF.js _pdfDoc in the onLoad callback, which would allow end-users to provide their own hooks in to the document. I wanted to explore the adding "find text" functionality to my viewer but as Angular-PDF keeps the document internal, it is impossible to extend currently.

if (angular.isFunction(scope.onLoad)) {
scope.onLoad(_pdfDoc);
}

Maybe also a new callback at the start of the renderPage function to allow end-users to perform their own extra logic/rendering too?

rhyskoedijk · 2017-04-09T00:10:39Z

Can I fork simobasso:es6 and submit my own pull requests for this new version, or is that just making things overly complicated? I am interested in:

Switching the canvasId to a canvasClassName. Class lookup rather than id looking works better when using multiple instances on the same page since you cannot generated unique canvas ids using angular bindings since the link function runs before any bindings in the template are resolved.
Exposing pdfDoc in the onLoad callback as an extension point for end-users
Exposing an onRender callback as an extension point for end-users

An issue I had around the pdfTask being cancelled when using multiple instances seems to be resolved in this version already as you have re-scoped the variables within the directive.

dennybiasiolli · 2017-04-09T06:10:23Z

Thanks for the feedback @rhyskoedijk, this speeds up our testing-before-merging process 😉

Our roadmap on the project is the following:

merge this PR in order to provide a better structured project
improve the project with the new features suggested

Starting from tomorrow we will fix conflicts in this PR in order to merge as soon as possible, so you can fork the master branch directly.
Your suggestions are great, after PR's merge we will glad to focus our attention to them and if you would like to contribute with PRs we can improve this directive together 😄

Thanks for your help

dennybiasiolli · 2017-04-11T09:57:08Z

webpack.config.js

+  publicPath: isProd ? '/' : 'http://localhost:8080/',
+  filename: "angular-pdf.min.js",
+  library: "pdf",
+  libraryTarger: "umd",


@simobasso I think this property is libraryTarget instead of libraryTarger

dennybiasiolli · 2017-04-11T10:03:45Z

package.json

-    "build": "grunt",
-    "start": "http-server ./example -p 8081",
-    "test": "grunt --verbose"
+    "build": "rimraf dist && webpack --progress --profile",


@simobasso can we install rimraf as devDependency?

dennybiasiolli · 2017-04-11T11:08:52Z

@simobasso what do you think about this branch?
It resolves conflicts, fixes sintax errors and provide both standard and minimized output.

rhyskoedijk · 2017-04-17T21:49:18Z

just merge it ;)

simobasso · 2017-04-18T12:10:58Z

close in favor of #184

dennybiasiolli reviewed Jan 2, 2017

View reviewed changes

webpack.config.js

presets: [

"latest",

],

"plugins": [ "istanbul" ]

Copy link

Collaborator

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use double quotes in property name

simobasso requested a review from dennybiasiolli January 2, 2017 14:26

dennybiasiolli reviewed Jan 2, 2017

View reviewed changes

webpack.config.js

},

emitError: false,

failOnError: false,

"rules": {

Copy link

Collaborator

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove this rule

dennybiasiolli reviewed Jan 2, 2017

View reviewed changes

webpack.config.js

},

jscs: {

validateIndentation: 2,

emitErrors: false,

Copy link

Collaborator

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please set true on emitError and failOnHint

dennybiasiolli reviewed Jan 2, 2017

View reviewed changes

dennybiasiolli reviewed Jan 4, 2017

View reviewed changes

travis: remove 0.10 test suite

25b02e1

node <= 0.xx doesn't support es6 syntax ref: http://node.green/

simobasso force-pushed the es6 branch 3 times, most recently from 0ecc917 to 16826dd Compare January 5, 2017 01:12

simobasso added 4 commits January 5, 2017 02:19

angular-pdf: rewrite entire module in es6

12acecc

karma: add coverage

9f11fa2

Generate code coverage using Istanbul

angular-pdf: add codecov report

ecdd2b2

simobasso force-pushed the es6 branch from 16826dd to ecdd2b2 Compare January 5, 2017 01:27

simobasso mentioned this pull request Jan 9, 2017

feat(*): allow AMD and CommonJS integration #168

Closed

dennybiasiolli mentioned this pull request Apr 8, 2017

Multiple instances within same page do not load (rhyskoedijk, issue #1) #182

Closed

dennybiasiolli mentioned this pull request Apr 11, 2017

Selectable text #183

Closed

dennybiasiolli reviewed Apr 11, 2017

View reviewed changes

dennybiasiolli mentioned this pull request Apr 18, 2017

Es6 #184

Merged

simobasso closed this Apr 18, 2017

simobasso deleted the es6 branch April 18, 2017 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Es6 #163

Es6 #163

simobasso commented Jan 2, 2017

codecov-io commented Jan 2, 2017 •

edited

Loading

dennybiasiolli Jan 2, 2017

dennybiasiolli Jan 2, 2017

dennybiasiolli Jan 2, 2017

dennybiasiolli Jan 2, 2017

dennybiasiolli Jan 2, 2017

dennybiasiolli Jan 4, 2017 •

edited

Loading

dennybiasiolli commented Jan 9, 2017

rhyskoedijk commented Apr 8, 2017

rhyskoedijk commented Apr 9, 2017

dennybiasiolli commented Apr 9, 2017

dennybiasiolli Apr 11, 2017

dennybiasiolli Apr 11, 2017

dennybiasiolli commented Apr 11, 2017

rhyskoedijk commented Apr 17, 2017

simobasso commented Apr 18, 2017

Es6 #163

Es6 #163

Conversation

simobasso commented Jan 2, 2017

codecov-io commented Jan 2, 2017 • edited Loading

Current coverage is 69.46% (diff: 69.46%)

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

dennybiasiolli Jan 2, 2017

Choose a reason for hiding this comment

dennybiasiolli Jan 4, 2017 • edited Loading

Choose a reason for hiding this comment

dennybiasiolli commented Jan 9, 2017

rhyskoedijk commented Apr 8, 2017

rhyskoedijk commented Apr 9, 2017

dennybiasiolli commented Apr 9, 2017

dennybiasiolli Apr 11, 2017

Choose a reason for hiding this comment

dennybiasiolli Apr 11, 2017

Choose a reason for hiding this comment

dennybiasiolli commented Apr 11, 2017

rhyskoedijk commented Apr 17, 2017

simobasso commented Apr 18, 2017

codecov-io commented Jan 2, 2017 •

edited

Loading

dennybiasiolli Jan 4, 2017 •

edited

Loading