Skip to content
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

FEATURE: Make collection export option dynamic #523

Merged
merged 2 commits into from
Jan 13, 2020
Merged

FEATURE: Make collection export option dynamic #523

merged 2 commits into from
Jan 13, 2020

Conversation

VladimirMikulic
Copy link
Contributor

@VladimirMikulic VladimirMikulic commented Jan 13, 2020

Demo

Resolves #522

@jywarren @sashadev-sky could you take a look at this? Thanks.

Copy link
Member

@sashadev-sky sashadev-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we set it to undefined then won't it overwrite whatever custom collection the user passed in on initialization and always set it to this._group.generateExportJson?

Anyway can we bump the version to 0.11.2 in package.json for when we fix this? And also the problem appears when the export completes too, not just on cancel so we want to make sure to reset the collection then too.

Thank you for your help with the exporter! Really appreciate it. I'll upload this to the dashboard

@VladimirMikulic
Copy link
Contributor Author

Thanks, @sashadev-sky. I've updated the PR.
If the user passes custom collection property, it won't reset it in cancelExport or when the export is completed. On the other hand, if the user hasn't specified the collection property it will reset it.

Copy link
Member

@sashadev-sky sashadev-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok awesome!! One more thing - should we remove the .images call at the end of JSON.stringify(opts.collection.images) and add it to the end of the generateExportJson calls instead, so that the user isn't forced to pass in their custom collection nested inside of an 'images' key?

From @jywarren's export.html file https://github.com/publiclab/Leaflet.DistortableImage/blob/main/examples/export.html it looks like we don't want to need that.

And I think you updated the Gruntfile.js file by accident. Also don't forget to bump package.json :)

@VladimirMikulic
Copy link
Contributor Author

@sashadev-sky thank you! I apologise for committing Gruntfile changes, it's not efficient to run tests and linter on every change(I disable them in development). That's why I am trying to incorporate Webpack in this project :)

As you suggested I attached the .images to the generateExportJson function.
It's a good idea not forcing users to pass collection inside of a .images property.

In the end, I had the honour to bump package.json 🎉

Copy link
Member

@sashadev-sky sashadev-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VladimirMikulic No prob! Yes you'll just want to run $ npm install $ npm install leaflet --no-save now so that the version updates in the package-lock.json and push that up too!

Changes:
 - New version 0.11.2
 - Rollback Gruntfile changes
 - .images call from JSON.stringify(opts.collection.images) attached to
generateExportJson
@VladimirMikulic
Copy link
Contributor Author

Thanks @sashadev-sky, I've implemented the required changes.

Copy link
Member

@sashadev-sky sashadev-sky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing!! Thank you so much for your help with the exporter, I need it!

CC @jywarren
and also @SidharthBansal this has been uploaded to GCI dashboard and @VladimirMikulic has completed it

@sashadev-sky sashadev-sky merged commit f6ef2c9 into publiclab:main Jan 13, 2020
@VladimirMikulic
Copy link
Contributor Author

@sashadev-sky anytime! More changes coming soon. 🚀

@jywarren
Copy link
Member

Oh my gosh already? My work over at publiclab/mapknitter#1192 is moving faster now, and I'm almost ready. Can you bump the package.json version by 0.0.1 and I can publish a new version? Thank you!

@VladimirMikulic
Copy link
Contributor Author

@jywarren are you referring to me or to @sashadev-sky?

@jywarren
Copy link
Member

Either, actually! If you open a PR with the version bump then either Sasha or I can run npm publish after merging it. Thanks!

@VladimirMikulic
Copy link
Contributor Author

#525 :)

@sashadev-sky
Copy link
Member

@VladimirMikulic The task is on GCI so make sure to claim it at some point :)

@VladimirMikulic
Copy link
Contributor Author

Thanks @sashadev-sky. I've seen it there. I'll claim it after the 'calculateProjectiveTransform test' gets approved :)

sashadev-sky pushed a commit to sashadev-sky/Leaflet.DistortableImage that referenced this pull request Jan 14, 2020
* FEATURE: Make collection export option dynamic

Resolves publiclab#522

* REFACTOR: Minor changes

Changes:
 - New version 0.11.2
 - Rollback Gruntfile changes
 - .images call from JSON.stringify(opts.collection.images) attached to
generateExportJson
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make collection export option dynamic
3 participants