[WIP] customizable export UI for DistortableCollection #485
Conversation
| @@ -8,9 +8,88 @@ L.DistortableCollection.Edit = L.Handler.extend({ | |||
|
|
|||
| initialize: function(group, options) { | |||
| this._group = group; | |||
|
|
|||
| // OK let's just refactor this to not use the closure scope, but to pass options into each function. | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
src/edit/actions/ExportAction.js
Outdated
| console.log(this, edit.options) | ||
| // explicitly send each option: | ||
| edit.startExport({ | ||
| collection: edit.options.collection, |
There was a problem hiding this comment.
Here we can see all the settable options being passed into the export start command; there are a lot, allowing for some really detailed customization of both the Export process management and the UI.
jywarren
force-pushed
the
custom-export
branch
from
778c16f
to
8ce836f
Compare
|
|
||
| // this is undefined at runtime but gets filled in later... is this an async issue? | ||
| // ok, wrapped this in a promise | ||
| console.log('options in startExport', opts, opts.exportUrl); |
There was a problem hiding this comment.
So, opts.exportUrl returns undefined but opts shows correct values, so we're seeing a timing or scope error here; this was the case also before adding the Promise code on line 16, which I had hoped would solve it. Could there be a scope collision with the opts parameter somehow?
There was a problem hiding this comment.
Solved this in latest code i'm pushing!
| collection: json, | ||
| // this needs to be a function run at the moment of pressing, not when initializing the collection | ||
| // scale: prompt("Choose a scale or use the default (cm per pixel):", 100), | ||
| exportUrl: 'http://34.74.118.242/api/v2/export/', // used to |
There was a problem hiding this comment.
Here I'm showing one reason WHY we want to be able to customize; we have alternative export servers with slightly different requirements, which we'd like to be able to connect to the Exporter in a simple standardized way.
|
@sashadev-sky any ideas on what I'm doing wrong here? My brain is a bit fried and I have a suspicion it's something pretty simple. |
|
@sashadev-sky can you please upload this task on the GCI dashboard by summarizing it. I am afraid I will not be able to properly summarise it as I don't have experience of LDI @jywarren should it be a hard task for GCI? |
jywarren
force-pushed
the
custom-export
branch
from
8309506
to
26c2065
Compare
|
Got this working! Now we need to handle the response! |
|
OK, it's basically working now! We overrode the actual images using a JSON collection, which we don't have to do, but demonstrates how we can filter the actual images submitted. We could further refine this but the key parts we need to wrap up a first implementation and merge it are:
Anything else? Then we should:
|
|
I passed the |
|
So perhaps the follow-ups could become GCI issues now? |
|
Hmm, I tried resolving the linting errors with reordering but may have made some mistakes. They are pretty minor though, once we resolve them we can merge this. |
|
All good here! @sashadev-sky can you give a final review after the holidays? 😄 |
|
We could also write a test against this by overriding the export functions and using a mock function to callback or something... interesting! |
|
Let's circle back to publiclab/mapknitter#1128 once we're done here, too! |
|
#501 adds tests on top of this but I wasn't able to get them to work... we can circle back later to finalize that! |
|
@jywarren there are a bunch of GCI PRs open right now for me to review! Let me know where this stands priority wise, assuming the others get precedence for now |
|
Hi, Sasha we need to review all the prs of GCI merged before GCI deadline.
…On Fri, 10 Jan 2020, 11:53 am Sasha Boginsky, ***@***.***> wrote:
@jywarren <https://github.com/jywarren> there are a bunch of GCI PRs open
right now for me to review! Let me know where this stands priority wise,
assuming the others get precedence for now
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#485?email_source=notifications&email_token=AFAAEQZZASZ5T3BOIIAOWLDQ5AH7LA5CNFSM4J6MRPKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIS2IYQ#issuecomment-572892258>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAAEQZBABYX5DYSW7TZNY3Q5AH7LANCNFSM4J6MRPKA>
.
|
|
Yes that I know for sure don’t worry! That’s not a prob |
|
This is actually really high priority if you're at all able to take a look. We are pretty urgently trying to get MapKnitter off of it's internal exporter to reduce the cloud server charges we incur from it. I'd appreciate if you could take a look! Thank you!! |
|
I think this is safe to merge and we can add the tests next. |
|
I'm going to go ahead and merge this, actually! It's got some key fixes needed in publiclab/mapknitter#1191 -- thank you!!! |
|
I'd still love a review however 😄 |
|
Awesome folks.
…On Sat, Jan 11, 2020 at 3:56 AM Jeffrey Warren ***@***.***> wrote:
Merged #485
<#485> into
main.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#485?email_source=notifications&email_token=AFAAEQ2ATTN4M6MKPXEH36DQ5DYYNA5CNFSM4J6MRPKKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOV462WFA#event-2940054292>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAAEQ46H7O3VQBP3GQL4W3Q5DYYNANCNFSM4J6MRPKA>
.
|
Fixes #478
cc @sashadev-sky @SidharthBansal this is very rough, but I'm making settable options params for all critical export UI and request/response handling; I'm seeing some issue with variable scope, however, so this needs a lot of debugging. I want to demo how setting a custom Exporter and Exporter UI work in the new examples/export.html file, too!
If we can get GCI folks engaged in this, that'd be amazing!