-
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
Restructure API #824
Restructure API #824
Conversation
Will soon complete the work and write the tests. |
@jywarren I have refactored the whole code. |
@jywarren please review. |
Divy123 can you push it to your own gh-pages branch so we can try it out? Thanks this is epic and impressive! Asking a review from @publiclab/is-reviewers!! |
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.
Awesome! This will be backwards incompatible so it can be released with the next major version v3.0.0
.
I am sorry for leaving some commented code behind. Deleting it. |
Sure, all you have to do is checkout |
Oh yes, you also have to activate github pages for the repo in the repo settings. Thanks. |
@jywarren it's hosted at https://divy123.github.io/image-sequencer/ |
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 is really impressive. The demo works properly for me, and the tests show very clearly that it runs fine and that the syntax is much simpler. I approve this but would love one more review from someone!
@harshkhandeparkar did this look good to you too? |
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.
A few changes suggested although I think I reviewed dist files so please make the required changes in the proper files. Sorry.
@harshkhandeparkar Made the said changes. @jywarren I think its ready to be merged now. |
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.
✔️ ready! You can add the label if you want
@jywarren please merge this. |
Wow, very exciting. Do we need to update the readme first? Shall we add changes there too? I agree this is ready to merge, though. Let's do the readme changes at the same time but it could be a separate PR if easier, or the same. What do you think? |
Surely I forgot to !! |
@jywarren Done with README!! |
@jywarren opinions please!! |
Hi @Divy123 just noting in my last 2 comments that |
@jywarren please look into the screenshot: |
@jywarren please look into. |
Ah ok perfect. Just checking. Merging this now!!!! |
Amazing work! Bumping to 3.0.0, breaking changes. Heads up @publiclab/is-reviewers !!! 🙌🏽😅🎉🎉🎉 |
Fixes #251
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/is-reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to 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!