Add sequencer.getSteps() #777
Conversation
test/core/modules/image-sequencer.js
Outdated
| @@ -165,6 +165,22 @@ test('insertSteps({image: {index: index, name: "module", o: options} }) inserts | |||
| }); | |||
|
|
|||
|
|
|||
| test('getSteps() returns correct array of steps', function(t){ | |||
| var stepsOptionsName = ['load-image', 'channel', 'channel', 'channel', 'channel', 'channel']; | |||
There was a problem hiding this comment.
This looks great!
Why don't we do this with a new instance of ImageSequencer so that we can control the exact steps passed in, and it isn't interdependent on prior tests? What do you think?
And we can also add this to the README!
There was a problem hiding this comment.
Sure.
Thanks for the review.
There was a problem hiding this comment.
Hi divy, the thing here is that we want to restructure this api in a way that the steps is not defined on the image but sequencer
I'll explain
Right now this function just creates an alias of sequencer.images.image1.steps but we want to make changes on the core such that sequencer.images.image1.steps does not exist but only sequencer.steps.
When that is done, out getSteps function will simply be
function getSteps(){
return sequencer.steps;
}|
Sure @tech4GT . |
|
Also one more thing @tech4GT that is image-sequencer deployment from some branch other than main as the link for the demo on the repo is different than what we the contributors are seeing in our local versions? |
|
Oh yes the gh-pages branch is what we deploy 😄 |
|
Yes I think we can do this first then restructure the internals in a separate PR. This looks good if someone else wants to approve too? And @Divy123 can you change the title to "add sequencer.getSteps()" so it's a bit more descriptive? Thanks! |
By this, @jywarren do you mean I should first do the core API part right and then work on getSteps() ? |
|
No sorry I meant this PR looks good to begin with and the core API can come
next, thanks!!
…On Sat, Feb 16, 2019, 5:37 AM Slytherin ***@***.*** wrote:
Yes I think we can do this first then restructure the internals in a
separate PR. This looks good if someone else wants to approve too? And
@Divy123 <https://github.com/Divy123> can you change the title to "add
sequencer.getSteps()" so it's a bit more descriptive? Thanks!
By this do you mean I should first do the core API part right and then
work on getSteps() ?
Thanks.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#777 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABfJxOZ5-EWjK92h_2jf25TKkcw02YRks5vN982gaJpZM4a9sge>
.
|
|
@jywarren please review. |
|
Awesome, great work!! |
Fixes #251
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test@publiclab/reviewersand@publiclab/is-reviewersfor help, in a comment belowIf tests do fail, click on the red
Xto learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!