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.

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

Allow custom ratio/width/height in fromDataURL #253

Merged
merged 2 commits into from
Jun 11, 2017
Merged

Allow custom ratio/width/height in fromDataURL #253

merged 2 commits into from
Jun 11, 2017

Conversation

halo
Copy link
Contributor

@halo halo commented May 16, 2017

Dear @szimek,

I know you're as busy as I am :) I'd still kindly ask you consider this PR as a small change that potentially solves a number of issues and PRs in one strike.

It simply allows a custom ratio, width, and height to be passed in when loading an image onto the canvas.

It should solve these Issues:

And you should be able to close these PRs:

If there is anything more I could do to assist, let me know.

Guessing the ratio is not a reliable solution
as can be seen by many Issues and pull requests regarding this.
This change is backwards compatible but greatly enhances flexibility.
@halo halo changed the title Can pass in custom ratio and size to fromDataURL Allow custom ratio/width/height in fromDataURL May 16, 2017
@halo halo changed the title Allow custom ratio/width/height in fromDataURL Allow custom ratio/width/height in #fromDataURL May 16, 2017
@halo halo changed the title Allow custom ratio/width/height in #fromDataURL Allow custom ratio/width/height in fromDataURL May 16, 2017
@mymattcarroll
Copy link
Contributor

👍

@halo
Copy link
Contributor Author

halo commented May 24, 2017

Yes, no, maybe? It would be sad if I had to uphold a fork. It's not a complex change. But if you don't have time in the next weeks, just let me know. So I won't have to follow up. Thank you!

(I'm a just little bit suspicious when I see issues and pull requests that are several years old :)

@szimek
Copy link
Owner

szimek commented May 24, 2017

Hey, huge thanks for this PR! In general I think it's a good idea, but I simply haven't got time yet to check it out. Hopefully, I'll find some time during this weekend.

@halo
Copy link
Contributor Author

halo commented May 24, 2017

Sure, just wanted to ask. No stress. I'm grateful you created this library in the first place! :D

@@ -92,11 +92,12 @@ SignaturePad.prototype.clear = function () {
this._isEmpty = true;
};

SignaturePad.prototype.fromDataURL = function (dataUrl) {
SignaturePad.prototype.fromDataURL = function (dataUrl, options) {
Copy link
Owner

Choose a reason for hiding this comment

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

After rewrite to ES6, you can probably set the default values for function parameters, so you could write it as

function (dataUrl, options = {}) {...}

and get rid of opts variable.

Copy link
Contributor Author

@halo halo May 24, 2017

Choose a reason for hiding this comment

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

Indeed. I wasn't sure so I followed the pattern i saw in other functions.

I changed it now, this is what it compiles to:

SignaturePad.prototype.fromDataURL = function (dataUrl) {
  var _this = this;

  var options = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};

  var image = new Image();
  var ratio = options.ratio || window.devicePixelRatio || 1;
  var width = options.width || this._canvas.width / ratio;
  var height = options.height || this._canvas.height / ratio;
  // ...

This compiles down to:

```
SignaturePad.prototype.fromDataURL = function (dataUrl) {
  var _this = this;

  var options = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};

  var image = new Image();
  var ratio = options.ratio || window.devicePixelRatio || 1;
  var width = options.width || this._canvas.width / ratio;
  var height = options.height || this._canvas.height / ratio;

```
@szimek szimek merged commit faf4cf7 into szimek:master Jun 11, 2017
@halo
Copy link
Contributor Author

halo commented Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants