Image Popup in MainImageModule IMPROVED:

[NitinBhasneria]

Fixes : #431

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 grunt jasmine
• code is in uniquely-named feature branch and has no merge conflicts
• PR is descriptively titled
• PR body includes fixes #0000-style reference to original issue #
• ask @publiclab/reviewers for help, in a comment below

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

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

[NitinBhasneria]

@VladimirMikulic @cesswairimu @jywarren My previous two PR's having the same problem in grunt jasmine. Please have a look.
Error is Fatal error: Callback must be a function

[VladimirMikulic]

@NitinBhasneria I've checked it out know. The failing tests are not your fault. mkdirp package uses object spread operator (...) which is not supported on Node < 8.3.
We run Travis on Node 5, 6, 7 and it fails.

[VladimirMikulic]

Please post a gif of your change.

[NitinBhasneria]

@VladimirMikulic while changing the things in the src folder in editor's code. The changes are reflected in example/index.html but it is not reflected in http://localhost:3000/post , even after grunt and yarn install --force. I also change path of editor in plots2/package.json.
Please guide me.

[VladimirMikulic]

You can simply copy the PublicLab Editor to the node_modules of plots2 and that's it.

[NitinBhasneria]

@VladimirMikulic I have copied the PublicLab.Editor in plots2/public/lib but still the changes are not reflected in http://localhost:3000/post

[NitinBhasneria]

In plots2/public/lib there is also a folder named publiclab-editor. Is there something to do with this too. I am really confused with these packages. Can you explain what to do?

[VladimirMikulic]

Correct, it's lib, not node_moduless, sorry.

[NitinBhasneria]

Bur still the changes are not reflecting. Can you help? @VladimirMikulic

[VladimirMikulic]

I don't have plots2 locally, but it if it doesn't work, it means that your code fails, which is most likely the case, since you assign a value in an if statement...

[NitinBhasneria]

Actually I even tried to give output in console but no output is shown.🙄

[VladimirMikulic]

Are you sure that you've compiled your changes and copied the old dist file with the updated one?

[NitinBhasneria]

Yes, I m sure. I have tried many times.

[VladimirMikulic]

I think I know why. Your JS is inside of HTML file which is not served on the client-side in plots2. Your change needs to be in .js file.

[NitinBhasneria]

Ok I will update you after further tests thanks

[NitinBhasneria]

Also I want to ask do I have to do yarn install after tests.

[VladimirMikulic]

You don't need yarn install since you manually copy files.

[NitinBhasneria]

Also there is pre existing publiclab-editor folder do er have to replace that wirh PublicLab.Editor folder?

[VladimirMikulic]

My zsh history says that the file that needs to be replaced is plots2/public/lib/publiclab-editor/dist/PublicLab.Editor.js

[NitinBhasneria]

Ok thanks, I will test and then update you surely

[NitinBhasneria]

@VladimirMikulic Thanks it worked.

[VladimirMikulic]

That's it @NitinBhasneria! Wohoo.

[NitinBhasneria]

@VladimirMikulic please have a look and approve this pr.

[NitinBhasneria]

@jywarren ready for your review

[keshav234156]

@NitinBhasneria I have addressed this issue #396 .I think we can close this issue

[jywarren]

Hi, just to disambiguate -- thanks @keshav234156 -- does this PR hide the "drag to upload" text whereas @keshav234156's PR preserves the image aspect ratio? So perhaps both are needed? Thanks a lot, if you can both help clarify exactly what your PR does, we can probably make use of both! Thanks!!! 🎉

[keshav234156]

@jywarren pr #396 fixes both of the issues aspect ratio as well as hiding of drag to drop text

[keshav234156]

This pr fixes the hiding of drag to upload text while uploading

[jywarren]

Ah, i'm sorry, i merged the other one first. Apologies for missing the parallel work here, I appreciate the contributions from everyone! @NitinBhasneria if you'd like to resolve conflicts and re-confirm that this works, I can merge it so everybody gets credited. Just be sure that the redundant parts of the code don't interfere destructively! Thank you @NitinBhasneria and @keshav234156 and @VladimirMikulic and @govindgoel!

[NitinBhasneria]

@jywarren done with conflicts. Thanks

[jywarren]

Oh hm, is this line duplicated form 136? And, would you mind uploading a GIF of this working just so we can be sure? Thank you!

[NitinBhasneria]

@jywarren Duplicate removed. here is the gif.

[jywarren]

Hooray! Thanks a huge amount. this is super great and thank you for sticking with it through a very careful and detailed review and revisions. Merging now!