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

Support External Images #81

Merged
merged 9 commits into from
Jul 28, 2017
Merged

Conversation

ccpandhare
Copy link
Collaborator

@ccpandhare ccpandhare commented Jul 26, 2017

Now browsers can load in external images onto the sequencer (or local ones) using just the path of the file. Earlier, it was required to provide a DataURL to Image Sequencer.

This can be done as long as CORS Restrictions aren't violated.

Should this be done via XMLHttpRequest? I don't see the benefit of doing that actually.

Discussion started in #78

@jywarren
Copy link
Member

jywarren commented Jul 26, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Suppose this is my file directory:

.
├── index.html
├── images
│   └── image.jpg
└── js
    └── image-sequencer.js

And now in index.html we can directly do the following, as far as CORS Restrictions don't dome into picture:

sequencer.loadImage('images/image.jpg');
sequencer.loadImage('http://domain/path/file.jpg');

I shall add a short explanation to README.md too

@jywarren
Copy link
Member

jywarren commented Jul 26, 2017 via email

@ccpandhare
Copy link
Collaborator Author

I have updated the README.

While we are at this, Should Image Sequencer allow Users on Node.js to directly load images from a URL?

@jywarren
Copy link
Member

jywarren commented Jul 27, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Done that. Some notes here:

  • Since images from URLs or large sized images take time to load, I have made it compulsary for loadImages(), loadImage() to get a callback. It can not be chained like before (all other method chainings are intact).

    This no longer works:

    sequencer.loadImage(URL).addSteps('step-name')

    This is how it is now:

    sequencer.loadImage(URL,function(){
        this.addSteps('step-name').run();
  • Images can be loaded directly from a URL now, like this:

    sequencer.loadImage('https://domain/path/to/image.png');

    The URL need not end in .jpg or .png. Any URL which returns a header with content-type: image/FORMAT will work.

  • I have documented all this in README.md

  • I have written a Unit Test to confirm this new feature works as expected.

@jywarren
Copy link
Member

Looks great! merge if you like, bump minor version number in keeping with SemVer - (new features).

Do data URLs still work synchronously? If not, and the callback is now required, that's a breaking API change, so we should release a v1.0.0 -- a major version bump. See semver.org for explanation! :-) this is great though!

@ccpandhare
Copy link
Collaborator Author

Thanks for all the kind words :-)

No, DataURLs don't work synchronously now because large DataURLs will cause a problem and can trigger addSteps before the image loads leading to a ReferenceError.

So do we call this a major update? We're still working on basic stuff, and specifications might change relatively rapidly, so maybe let's just wait for publishing a new version... What do you think about this? If we don't publish it it, we don't have to bump the version right?

Sorry if I am being naïve here, I haven't handled versions before. These are just my thoughts...

@ccpandhare ccpandhare changed the title Support External Images via Canvas Support External Images Jul 28, 2017
@jywarren
Copy link
Member

jywarren commented Jul 28, 2017 via email

@ccpandhare
Copy link
Collaborator Author

Yep! Okay!
Merging!

@ccpandhare ccpandhare merged commit 8610ab1 into publiclab:master Jul 28, 2017
ccpandhare added a commit to ccpandhare/image-sequencer that referenced this pull request Jul 28, 2017
@jywarren jywarren mentioned this pull request Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants