-
Notifications
You must be signed in to change notification settings - Fork 209
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
replaceImage method as requested in #32 #35
Conversation
Should I merge this Is, @jywarren ? |
I think we need to say "imageId" instead and use getElementById, and why do we need newSrc? Shouldn't it just run the sequencer on the given image and replace it with the output? Thanks!!! |
I am afraid not... |
OK, so what i mean is, if you have an image on a webpage, what's everything you need to do in ImageSequencer to have a modified image replace that image? You need the I notice, with chaining methods, (#40), you could do: $('#myImage').replace(sequencer.addStep(...).run($('#myImage')[0]));
// but an example not in jQuery, maybe |
Okay. I thing the both of us have different understandings of what
`replaceImage` should do.
According to me:
Given an already present sequencer, the method changes the initial image,
thus, applying the same stpes to a completely different image and resulting
to a sequencer with initial image as taken from the user and steps
inherited from the previous image.
I believe you think otherwise. Is that so?
…On बुध, ५, जुलै २०१७, २३:४४ Jeffrey Warren ***@***.***> wrote:
OK, so what i mean is, if you have an image on a webpage, what's
everything you need to do in ImageSequencer to have a modified image
replace that image? You need the img element's id so you can find it, and
then you need some steps to run on it. The final state of hte image (after
all the steps have run) should then be inserted back into the webpage,
replacing the original.
I notice, with chaining methods, (#40
<#40>), you could do:
$('#myImage').replace(sequencer.addStep(...).run($('#myImage')[0])); // but an example not in jQuery, maybe
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT0xnCmsom_G881GX_bFy1atSbVEfKkAks5sK9JrgaJpZM4OLYk5>
.
|
Also, what you said will not be there for Node.js right?
|
Now I do get what you were saying. |
Right, not for node -- and this is a slightly different use of IS -- the only state would be the steps, not the image. So we use it as if it were a filter for any given image. I guess in that usage, we might not want to actually store the image, or affect any images already loaded into the sequencer -- this is more just a useful way to use a sequencer that's already been set up, to do things on a webpage. |
So do you mean that a separate mode should be made in which the intermediate images aren't stored? Or maybe they're deleted shortly after? Is it a shorthand for Is it a shorthand for What exactly does it replace? (an image with an image in the DOM) or (an image with an image in the ImageSequencer) ? |
I think we could leave the intermediate images in place and if we never use the sequencer instance again, they'll just get abandoned, which for this use is fine. It is a shorthand for the first option -- the intention being to take an image from the DOM, run steps on it, and insert it back into the DOM where the original was. Imagine a use case -- you want to use IS to make every image in a web-based gallery monochrome. You could run (roughly): $('img.galleryImages').each(function(image) {
var s = new ImageSequencer();
s.addStep('monochrome').run(image, function onComplete(output) { $(image).replace(output) });
}); or in shortened syntax (and for just one image, in this case) something really minimal, like: ImageSequencer.replaceImage('img#myImage', steps); I'm actually agnostic about whether we should use an existing instance of sequencer with steps already in it, or we should require steps to be added, and treat this as a kind of "quick constructor" -- that's up to you. I am mostly thinking about a convenient and compact use case and how to encourage people to easily use ImageSequencer in their web-based projects. Imagine wanting to make a quick JS utility to crop every image on a page, or generate thumbnails, or whatever -- what's the simplest and most straightforward way for IS to help people do this? |
Wow! That is a great insight, I must say! |
Super, and sorry if I was mixing up the architectural question of state and so forth with the real desire which was just to increase adoption with an easy to use and short function! :-) |
No, what you said is perfectly fine! |
Latest Changes
|
if(url.substr(0,11).toLowerCase()!="data:image/") xmlHTTP.send(); | ||
else make(url); | ||
|
||
function make(url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the pros/cons of using this workaround here vs. just letting a canvas handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PROS:
- May bypass some CORS restrictions.
- Will also work on browsers not supporting Canvas. (read: IE < 9)
CONS:
- An extra HTTP request each time.
I feel this needs more testing (to find out whether the premise of bypassing CORS restrictions is actually valid). What do you say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. But I also like the nonrequirement of canvas, though of course some modules will decide to use it.
Cool, love the demo idea! Let's merge this as soon as we can as it's getting a bit complex and we really should be doing separate PRs for separate features. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess i think we need to start testing LoadImage and ReplaceImage, and in general step up our testing, otherwise we'll get into a state where getting closer to full test coverage starts to seem overwhelming! What do you think? You could test that ReplaceImage changes the src
of the images, you could actually resize them and then check their widths, maybe, to confirm that it happened? Like... could we test it with $('image').width() == 100
after or something.
Do we already test LoadImage?
Great work here! Exciting!
README.md
Outdated
`selector` is a CSS selector. If it matches multiple images, all images will be | ||
modified. `steps` may be the name of a module or array of names of modules. | ||
|
||
Note: Local images will work only if they are in the same directory or a subdirectory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local images in the browser? why is this? is it relative path required? i.e. ../../
then we could say that. This is a bit mysterious sounding without an explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose
the HTML file is in /Users/PublicLab/Desktop/image-sequencer
, also:
image1 : <img src="image.jpg">
image2 : <img src="images/image.jpg">
image3: <img src="../image.jpg">
image4: <img src="/Users/PublicLab/Desktop/image.jpg">
Then:
image1, image2 are allowed.
image3, image4 will throw a CORS Error on some browsers like those running on Gecko.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated README.md with a better explanation.
README.md
Outdated
Note: Local images will work only if they are in the same directory or a subdirectory. | ||
|
||
```js | ||
sequencer.replaceImage(selector,steps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you show what steps
should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I shall add that.
replace-demo.html
Outdated
@@ -0,0 +1,63 @@ | |||
<!DOCTYPE html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this go in the examples folder, as just replace.html
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry.
Also -- push a copy of this branch up to your gh-pages branch so we can try your demo! at https://ccpandhare.github.com/image-sequencer/examples/replace |
Yes. I will add more intensive tests for |
The demo is now live at https://ccpandhare.github.io/image-sequencer/examples/replace ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Merge?
Can options be set in steps on replaceImage? We don't have to, just wondering. Can we say in the docs that steps is just passed to addsteps, so usage is the same?
Yes, options can be added. I can work on it. It is not exactly the same, as I haven't added support for options yet. It will hardly take a while. I will add that before merging. Is that okay? Also this is my first demo for Image Sequencer! Do you like it? :-) |
Oh exciting! It works perfectly and looks great. Kudos!
…On Jul 8, 2017 9:46 AM, "Chinmay Pandhare" ***@***.***> wrote:
The demo is now live at https://ccpandhare.github.io/
image-sequencer/examples/replace.html !
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ-lfnT1fNm8HSVDcjaKatH4o_S6Kks5sL4gWgaJpZM4OLYk5>
.
|
Aha, very interesting! If you repeat clicking too long, the image degrades; we can address this in a follow-up issue but it's really cool to see, actually! Must be decimal/integer issues. Yes go ahead and tweak the steps parameter as I think it's worth it for it to work the same way as addsteps, and you might even consider just passing it to addsteps to reduce code? |
Thanks! Wow! Interesting!
That's exactly whats happening. This is the code behind this_.loadImage('default',url).addSteps('default',steps,options).run(function(out){
the_image.src = out;
}); |
Oh perfect. Super!
…On Jul 8, 2017 10:00 AM, "Chinmay Pandhare" ***@***.***> wrote:
Oh exciting! It works perfectly and looks great. Kudos!
Thanks!
Wow! Interesting!
Actually this has to do with save-pixels.
save-pixels doesn't save images with 100% quality by default. That's why
the image quality degrades with every use.
and you might even consider just passing it to addsteps to reduce code?
That's exactly whats happening. This is the code behind replaceImages:
this_.loadImage('default',url).addSteps('default',steps,options).run(function(out){
the_image.src = out;
});
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#35 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ4_PgyNWJmL2y_mmauim87ATQIoCks5sL4twgaJpZM4OLYk5>
.
|
We can simply pass |
Yes, mentioning that in the docs can be a succinct and clear way to show
proper usage. +1!
…On Jul 8, 2017 10:01 AM, "Jeffrey Warren" ***@***.***> wrote:
Oh perfect. Super!
On Jul 8, 2017 10:00 AM, "Chinmay Pandhare" ***@***.***>
wrote:
> Oh exciting! It works perfectly and looks great. Kudos!
>
> Thanks!
>
> Wow! Interesting!
> Actually this has to do with save-pixels.
> save-pixels doesn't save images with 100% quality by default. That's why
> the image quality degrades with every use.
>
> and you might even consider just passing it to addsteps to reduce code?
>
> That's exactly whats happening. This is the code behind replaceImages:
>
> this_.loadImage('default',url).addSteps('default',steps,options).run(function(out){
> the_image.src = out;
> });
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <#35 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AABfJ4_PgyNWJmL2y_mmauim87ATQIoCks5sL4twgaJpZM4OLYk5>
> .
>
|
Build Passed, merging it in! |
🎈 |
What Changes this PR Contains
replaceImage
Run.js
Readme.md
replaceImage
replaceImage Syntax