-
Notifications
You must be signed in to change notification settings - Fork 208
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
Implemented UI Methods #67
Conversation
A user can set their own UI as follows: sequencer.setUI({
setup: function(){...} ,
drawing: function(){...} ,
drawn: function(){...} ,
remove: function(){...}
}); If the user does not do so, the default UI is applied. Also, for each module, an object (
|
I think, using this information, One can create a UI with ease. |
Can you write the readme section for this? That way I can follow more
easily too. Perhaps some tests demonstrating the UI callbacks?
Sorry im on not great internet so tell me if you've already done that!
It looks amazing!
On Jul 21, 2017 10:53 PM, "Chinmay Pandhare" <notifications@github.com> wrote:
I think, using this information, One can create a UI with ease.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ-eEL0_X2yJNqJdKRbh6GdXAYZWZks5sQQ-ngaJpZM4Of34C>
.
|
Ohh... I see. No I haven't written the documents yet, I wanted to show it to you first and confirm if this looks okay. But I shall Add the documentation now. As far as the demo is concerned, I have already switched all control to this module. That is, Now whatever Console.logs happen, are via this module! And I am logging at all these events. This is a screenshot of by browser console running a test sequence: |
Also, I am changing the method names to what you had suggested originally. |
I have Added the documentation to Also I have changed the method names to those you had suggested in #65 These are the name changes:
|
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 fantastic! I left some questions, comments, requests. Great work!
identity = { | ||
stepName: "Name of the Step", | ||
stepID: "A unique ID given to the step", | ||
imageName: "The name of the image to which the step is added." |
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.
Filename?
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.
Umm, no...
To handle multiple images, We needed a unique identification for the images, which would also be accessible to the user. So I came up with image names back in Phase 1 (#24) .
Every image has a "name". this name is either defined by the user while loading an image:
sequencer.loadImage("name","SRC");
If the user does not specify any name, that is:
sequencer.loadImage("SRC");
Then Image Sequencer gives the image a name on its own, like "image1" or "image2" etc. This feature was discussed and implemented in #48 , implemented in #53
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.
cool, i remember. Maybe for forgetful ppl like me or for newcomers we could make a quick reference to the explanation from the identity
documentation. Thanks!
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 fixed this in a commit, thanks!
|
||
```js | ||
sequencer.setUI({ | ||
onSetup: function() {}, |
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.
Should these have access (by parameter like onSetup(image, metadata)
to the image itself so that its more explicit and the function can be written generically but applied to each image separately? We should show the right way to access these values even if we don't pass them via parameter but that makes it really clear. Thoughts?
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.
Should we ask to return the generated html element if any?
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 I think it already is what you are talking about. Generic functions can be written.
The user has access to :
- What image the step is applied to
- Details of the step
- Details of the client : browser/Node
This is all the data which is needed to make a UI, I feel.
Although we could give access to the original image, and the metadata, but I am not really clear about what you are saying there...
I don't think we should ask to return generated HTML. That is because, we don't really need it. The user has all access to the front-end of the code. We are providing all the data the user might need and he can implement it to the front-end as he wishes.
```js | ||
sequencer.setUI({ | ||
onSetup: function() {}, | ||
onDraw: function() {}, |
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.
Do we need to pass the HTML of the element produced (if any) in onSetup?
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 don't think that might be required... I see that there's some misunderstanding here, I shall explain what I am saying in a comment on the main thread and we will come to a conclusion later!
src/UserInterface.js
Outdated
UI.onSetup = UI.onSetup || function() { | ||
if(options.ui == "none") { | ||
// No UI | ||
} |
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.
Would it work to use the value false
?
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, it would.
Actually for the unit tests, we started doing this:
var sequencer = ImageSequencer({ui:"none"});
This is where it originated, I also feel we should make it false
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 fixed this in a commit, thanks!
src/modules/Crop/Module.js
Outdated
var output | ||
|
||
function draw(input,callback) { | ||
|
||
UI.onDraw(); | ||
const this_ = this; | ||
|
||
require('./Crop')(input,options,function(out,format){ | ||
this_.output = { |
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.
This doesn't error on removing var this_ = this
?
Also, best practice is to use a meaningful name like module
instead of this_. Thanks!
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, you are right...
I had actually gotten into the practice of doing this from writing callbacks (and avoiding the scope change problem of this
):
function x() {
var y = 1;
var this_ = this;
someObj.someMethod(abc, function() {
this_.y = 2;
});
}
I will change it. Thanks!
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 fixed this in a commit, thanks!
OK, i'm satisfied with responses here, thanks for your thoughtful replies! Perhaps a little more documentation can help clarify this for new folks, but i think it's ready to merge if you do. I'm going to start publishing this on NPM, calling the current version |
https://www.npmjs.com/package/image-sequencer at :-) |
Yes, sounds good! Thanks! :-) sequencer.setUI({
onSetup: function() {
// create div (pseudocode)
var div = "<div id=${identity.stepID}><img src='' /></div>"
// append to the container representing the image (pseudocode)
document.querySelector('#image-sequencer-${identity.imageName}').append(div);
},
onDraw: function() {
// overlay a loading GIF over the child #${identity.stepID} of the container #image-sequencer-${identity.imageName}
},
onComplete: function(output) {
// remove the overlay
// update the image element in #${identity.stepID} with the new SRC
},
onRemove: function() {
// remove the appended DIV
}
}); This is what I have implemented yet. I hope this makes stuff a bit clear. I shall improve the documentation for this. |
+1!
…On Jul 24, 2017 8:04 PM, "Chinmay Pandhare" ***@***.***> wrote:
Yes, sounds good! Thanks! :-)
I would like to make a few changes though, the ones you have suggested
above.
And just to explain the usage:
sequencer.setUI({
onSetup: function() {
// create div (pseudocode)
var div = "<div id=${identity.stepID}><img src='' /></div>"
// append to the container representing the image (pseudocode)
document.querySelector('#image-sequencer-${identity.imageName}').append(div);
},
onDraw: function() {
// overlay a loading GIF over the child #${identity.stepID} of the container #image-sequencer-${identity.imageName}
},
onComplete: function(output) {
// remove the overlay
// update the image element in #${identity.stepID} with the new SRC
},
onRemove: function() {
// remove the appended DIV
}
});
This is what I have implemented yet. I hope this makes stuff a bit clear.
I shall improve the documentation for this.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJwpHFiBAJKidWEAMqHhO7yPo4W1Sks5sRNy2gaJpZM4Of34C>
.
|
This looks ready to me now! Merging this in! |
Super! Do you have ability to create a new release?
…On Jul 24, 2017 11:06 PM, "Chinmay Pandhare" ***@***.***> wrote:
Merged #67 <#67>.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJ6Se-pIm3vp6Jl8Kq0qHOFcP-XZNks5sRQdIgaJpZM4Of34C>
.
|
Sorry, I don't think I have that access.
On मंगळ, २५, जुलै २०१७, १७:०१ Jeffrey Warren <notifications@github.com>
wrote:
… Super! Do you have ability to create a new release?
On Jul 24, 2017 11:06 PM, "Chinmay Pandhare" ***@***.***>
wrote:
> Merged #67 <#67>.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#67 (comment)>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/AABfJ6Se-pIm3vp6Jl8Kq0qHOFcP-XZNks5sRQdIgaJpZM4Of34C
>
> .
>
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AT0xnF_XfLWe5hpkm-ucNie71E-J_FkBks5sRdIOgaJpZM4Of34C>
.
|
Ok I will soon. Thanks!
On Jul 25, 2017 1:33 PM, "Chinmay Pandhare" <notifications@github.com>
wrote:
… Sorry, I don't think I have that access.
On मंगळ, २५, जुलै २०१७, १७:०१ Jeffrey Warren ***@***.***>
wrote:
> Super! Do you have ability to create a new release?
>
> On Jul 24, 2017 11:06 PM, "Chinmay Pandhare" ***@***.***>
> wrote:
>
> > Merged #67 <#67>.
> >
> > —
> > You are receiving this because you commented.
> > Reply to this email directly, view it on GitHub
> > <#67 (comment)
>,
> > or mute the thread
> > <
> https://github.com/notifications/unsubscribe-auth/AABfJ6Se-
pIm3vp6Jl8Kq0qHOFcP-XZNks5sRQdIgaJpZM4Of34C
> >
> > .
> >
>
> —
> You are receiving this because you modified the open/close state.
>
>
> Reply to this email directly, view it on GitHub
> <#67#
issuecomment-317709875>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AT0xnF_XfLWe5hpkm-
ucNie71E-J_FkBks5sRdIOgaJpZM4Of34C>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#67 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJyLvvmmEM3lfz0kDQGaAF5CttlZOks5sRdKQgaJpZM4Of34C>
.
|
I have implemented basic UI Methods.
UI
UI
has four methods:setup
drawing
drawn
remove
setup
is called when the step is addeddrawing
is called whenmodule.draw
is calleddrawn
is called when the step produces outputremove
is called whensequencer.removeStep
is called on that step.Currently, The UI just triggers a
console.log($DATA)
.