[GCI] InsertStep functionality added for loadImage step #1360

harshkhandeparkar · 2019-12-15T14:25:43Z

Fixes #1300 (<=== Replace 0000 with the Issue Number)
Fixes #1363

As I mentioned in the issue thread, I fixed the whole insert step function which was broken badly.

Here's what was broken:

When the insert step button is clicked, it generates the previews in the last step instead of the step that is clicked(This was an insertPreviews.js bug)

Fixed

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
code is in a uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
ask @publiclab/is-reviewers for help, in a comment below
Insert-step functionality is working correct as expected.

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If 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
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part

Thanks!

codecov · 2019-12-15T14:29:49Z

Codecov Report

Merging #1360 into main will decrease coverage by 0.18%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##             main    #1360      +/-   ##
==========================================
- Coverage   66.41%   66.23%   -0.19%     
==========================================
  Files         125      125              
  Lines        2552     2559       +7     
  Branches      397      400       +3     
==========================================
  Hits         1695     1695              
- Misses        857      864       +7

Impacted Files	Coverage Δ
examples/lib/intermediateHtmlStepUi.js	`11.11% <0%> (-0.26%)`	⬇️
examples/lib/defaultHtmlStepUi.js	`10.98% <0%> (-0.4%)`	⬇️
examples/lib/insertPreview.js	`13.51% <28.57%> (ø)`	⬆️

harshkhandeparkar · 2019-12-15T14:41:05Z

This was supposed to be a one or two file PR but I ended up changing 6 files.

harshkhandeparkar · 2019-12-15T14:42:40Z

This ended up being more complicated than I had initially guessed.

keshav234156 · 2019-12-15T18:04:10Z

@harshkhandeparkar Can you please open a separate issue and PR for that?

harshkhandeparkar · 2019-12-15T18:05:46Z

But then the two are related. The insert step for loadImage depends upon the fix. Without the fix, loadImage cannot have insertStep functionality. What do you suggest I do ?

…

On Sun, 15 Dec, 2019, 11:34 PM keshav234156, ***@***.***> wrote: @harshkhandeparkar <https://github.com/HarshKhandeparkar> Can you please open a separate issue and PR for that? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360?email_source=notifications&email_token=AIJI5H2UAWNKRZGM5DNILYLQYZWRXA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG46SOI#issuecomment-565831993>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJI5HYS6KUPZCCXP2GLO5LQYZWRXANCNFSM4J3ALRAQ> .

keshav234156 · 2019-12-15T18:09:35Z

ok then i will open a new task please claim that also.

harshkhandeparkar · 2019-12-15T18:11:18Z

Uh ok. Do you want me to still open a new pr? Will it be easier to review that way?

…

On Sun, 15 Dec, 2019, 11:39 PM keshav234156, ***@***.***> wrote: @ok <https://github.com/ok> then i will open a new task please claim that also. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360?email_source=notifications&email_token=AIJI5H32IEWZ3ELNN5Q3DZTQYZXGDA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG46V5Y#issuecomment-565832439>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJI5HZQAUP47PXYA63XWVTQYZXGDANCNFSM4J3ALRAQ> .

keshav234156 · 2019-12-15T18:12:24Z

If you can. then go for it

harshkhandeparkar · 2019-12-15T18:15:33Z

It will be really difficult though. I will have to get the first pr merged while hiding the second one. Once the first one is merged, I will have to rebase the second, get it reviewed, get it merged.... It will be difficult for you too. If it is possible to review a single PR then please do so.

…

On Sun, 15 Dec, 2019, 11:42 PM keshav234156, ***@***.***> wrote: If you can. then go for it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360?email_source=notifications&email_token=AIJI5H2AE7JDGEK7HAHJ7WDQYZXQTA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG46YAI#issuecomment-565832705>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJI5H7IZKU3XB5PHAIIUOLQYZXQTANCNFSM4J3ALRAQ> .

keshav234156 · 2019-12-15T18:18:15Z

It will be really difficult though. I will have to get the first pr merged while hiding the second one. Once the first one is merged, I will have to rebase the second, get it reviewed, get it merged.... It will be difficult for you too. If it is possible to review a single PR then please do so.
…
On Sun, 15 Dec, 2019, 11:42 PM keshav234156, @.***> wrote: If you can. then go for it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360?email_source=notifications&email_token=AIJI5H2AE7JDGEK7HAHJ7WDQYZXQTA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG46YAI#issuecomment-565832705>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIJI5H7IZKU3XB5PHAIIUOLQYZXQTANCNFSM4J3ALRAQ .

ok,i understand that .Will do the review in single PR

harshkhandeparkar · 2019-12-15T18:20:14Z

Thanks. On Sun, 15 Dec, 2019, 11:48 PM keshav234156, <notifications@github.com> wrote:

…

It will be really difficult though. I will have to get the first pr merged while hiding the second one. Once the first one is merged, I will have to rebase the second, get it reviewed, get it merged.... It will be difficult for you too. If it is possible to review a single PR then please do so. … <#m_6771268481377171796_> On Sun, 15 Dec, 2019, 11:42 PM keshav234156, *@*.***> wrote: If you can. then go for it — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360 <#1360>?email_source=notifications&email_token=AIJI5H2AE7JDGEK7HAHJ7WDQYZXQTA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG46YAI#issuecomment-565832705>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIJI5H7IZKU3XB5PHAIIUOLQYZXQTANCNFSM4J3ALRAQ . ok,i understand that .Will do the review in single PR — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1360?email_source=notifications&email_token=AIJI5H6MTLZQY4GK546GLKTQYZYGRA5CNFSM4J3ALRA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG463MQ#issuecomment-565833138>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIJI5HYSGYH5QF5VY5H2D6DQYZYGRANCNFSM4J3ALRAQ> .

keshav234156

1.insert step in load image is not present once you delete any step.please try to figure out.

examples/lib/insertPreview.js

examples/demo.js

keshav234156 · 2019-12-16T16:09:46Z

1.insert step in load image is not present once you delete any step.please try to figure out.

harshkhandeparkar · 2019-12-16T16:12:47Z

1.insert step in load image is not present once you delete any step.please try to figure out.

What? I didn't get you.

harshkhandeparkar · 2019-12-16T16:14:24Z

Ok ok, I found the error. Fixing it now.

keshav234156 · 2019-12-16T16:24:18Z

@harshkhandeparkar I couldn't get why previews were not displayed in the second step ie where were we wrong.

harshkhandeparkar · 2019-12-16T16:31:27Z

I managed to fix this

1.insert step in load image is not present once you delete any step.please try to figure out.

harshkhandeparkar · 2019-12-16T16:33:14Z

@harshkhandeparkar I couldn't get why previews were not displayed in the second step ie where were we wrong.

That is because the previews were being added directly to the last step's dropdown instead of adding to the dropdown which is clicked.

harshkhandeparkar · 2019-12-16T16:40:18Z

@keshav234156 I opened the new issue on request.

jywarren · 2019-12-16T16:54:29Z

Aha! Cool, thanks for working through this. I think it needs an update (i can press the button if you like?) but just ping me when it's ready for merge and we can prioritize this! Thank you!

harshkhandeparkar · 2019-12-16T16:56:20Z

This needs a mentor review, can we merge the comments PRs before this? Rebasing will be simple.

jywarren · 2019-12-16T16:56:50Z

Comments PR? Can u point me to it? Thanks!

harshkhandeparkar · 2019-12-16T16:58:30Z

PR #1343 to #1351 which are parts of the HoF task.

keshav234156

Great Work!!!!!!

keshav234156 · 2019-12-16T17:09:06Z

@jywarren we can merge this as well before update

harshkhandeparkar · 2019-12-16T17:09:48Z

Thanks, @keshav234156. Should I add the label?

harshkhandeparkar · 2019-12-16T17:10:55Z

is-reviewers had tried to make this a rule - always sort PRs through labels (Work-In-Progress, Needs Work, Ready-To-Merge, Small, Critical, Important, etc)

jywarren · 2019-12-16T17:56:21Z

OK, now if we update this it should pass and we can merge 🙌

* add markuo * add markupfix insertPreview * fix insertPreview test * make the new insertPreview work * fix disabled * small fix * fix comments * remove console log * fix enabling/disabling of insert-step btn

Mr. X added 6 commits December 15, 2019 19:41

add markuo

c65be7d

add markupfix insertPreview

9089ad1

fix insertPreview test

11494e2

make the new insertPreview work

06c88b2

fix disabled

9b85865

small fix

9806514

harshkhandeparkar requested review from SidharthBansal and a team December 15, 2019 14:37

keshav234156 requested changes Dec 15, 2019

View reviewed changes

examples/lib/insertPreview.js Outdated Show resolved Hide resolved

examples/demo.js Outdated Show resolved Hide resolved

examples/demo.js Outdated Show resolved Hide resolved

Mr. X added 2 commits December 16, 2019 21:36

fix comments

0439702

remove console log

d758e31

harshkhandeparkar requested a review from keshav234156 December 16, 2019 16:07

fix enabling/disabling of insert-step btn

f2d89b4

harshkhandeparkar mentioned this pull request Dec 16, 2019

Insert Step Previews not Working as Expected #1363

Closed

Merge branch 'main' into insert-btn-in-step`

ec71746

keshav234156 approved these changes Dec 16, 2019

View reviewed changes

keshav234156 added the ready-to-merge label Dec 16, 2019

Merge branch 'main' into insert-btn-in-step`

451081c

jywarren approved these changes Dec 16, 2019

View reviewed changes

jywarren merged commit 4a86abb into publiclab:main Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GCI] InsertStep functionality added for loadImage step #1360

[GCI] InsertStep functionality added for loadImage step #1360

harshkhandeparkar commented Dec 15, 2019 •

edited

Loading

codecov bot commented Dec 15, 2019 •

edited

Loading

harshkhandeparkar commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019 •

edited

Loading

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 left a comment

keshav234156 commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

keshav234156 commented Dec 16, 2019 •

edited

Loading

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

keshav234156 left a comment

keshav234156 commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

[GCI] InsertStep functionality added for loadImage step #1360

[GCI] InsertStep functionality added for loadImage step #1360

Conversation

harshkhandeparkar commented Dec 15, 2019 • edited Loading

Fixed

codecov bot commented Dec 15, 2019 • edited Loading

Codecov Report

harshkhandeparkar commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019 • edited Loading

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 commented Dec 15, 2019

harshkhandeparkar commented Dec 15, 2019 via email

keshav234156 left a comment

Choose a reason for hiding this comment

keshav234156 commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

keshav234156 commented Dec 16, 2019 • edited Loading

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

keshav234156 left a comment

Choose a reason for hiding this comment

keshav234156 commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

harshkhandeparkar commented Dec 16, 2019

jywarren commented Dec 16, 2019

harshkhandeparkar commented Dec 15, 2019 •

edited

Loading

codecov bot commented Dec 15, 2019 •

edited

Loading

keshav234156 commented Dec 15, 2019 •

edited

Loading

keshav234156 commented Dec 16, 2019 •

edited

Loading