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

Separate each tool into its own file, starting with "toggleExport" #161

Closed
jywarren opened this issue Feb 28, 2019 · 6 comments
Closed

Separate each tool into its own file, starting with "toggleExport" #161

jywarren opened this issue Feb 28, 2019 · 6 comments

Comments

@jywarren
Copy link
Member

Currently all the different tools live in this file:

https://github.com/publiclab/Leaflet.DistortableImage/blob/main/src/edit/DistortableImage.Edit.js

I think it makes sense to create a new folder such as:

/src/edit/tools/

and have each tool be a separate file like:

/src/edit/tools/export.js
/src/edit/tools/showToolbar.js

etc. However, this library is not compiled using browserify or webpack and so we can't use require() to do this.

Is it:

  1. to add the tool methods to a subobject of Leaflet, like L.DistortableImage.EditTools?
  2. to refactor this library to start using require (a lot of work... maybe not worth it)

In either case, we'll have to pass in some context to the tools; most ask for access to this, which in this case is the L.DistortableImage.Edit context. We could pass this in like so:

L.DistortableImage.EditTools = L.DistortableImage.EditTools || {};

L.DistortableImage.EditTools.Export = function(edit) {
  function tool() {
    // then we can call it as we had been using `this`:
    // so this: this._lockHandles = new L.LayerGroup();
    // becomes:
    edit._lockHandles = new L.LayerGroup();
  }
  return tool; 
}

Then back in the original DistortableImage.Edit.js file, we'd bind it back in like this:

_toggleExport: L.DistortableImage.EditTools.Export(this);

The only thing left is that we'd need to add each tool to the concatenation script here:

src: [
'src/util/*.js',
'src/edit/EditHandle.js',
'src/edit/LockHandle.js',

This is a bit convoluted but should result in a simpler set of source files for the tools, where each file represents only a single function.

@rexagod @sashadev-sky what do you think of this?

@sashadev-sky
Copy link
Member

@jywarren I think that most modern rails applications use webpack and it wouldn't be difficult to set up a config file. I think we should go for the former because it is encapsulated and clear about what its point is. Would be to take this up once i get the exporter system for Mapknitter running

@rexagod
Copy link
Member

rexagod commented Apr 11, 2019

@jywarren Let's pin this one! That would ensure that we abide by this approach. What to you think?

@sashadev-sky sashadev-sky pinned this issue Apr 12, 2019
@sashadev-sky
Copy link
Member

@rexagod pinned!

@sashadev-sky sashadev-sky mentioned this issue Apr 13, 2019
7 tasks
@sashadev-sky
Copy link
Member

adding this here as it explains the end goal that this PR is working towards --

this is branched out of #140

"We have a general need to brainstorm new UI designs for tools as more tools are added. Maybe nested menus or a modal for more tools, as the menu runs out of space?

But to explore this, I'd like to propose that we separate the tools UI from the tool functions themselves.

By abstracting the tools UI from tools functions, we'd allow for alternative tools UIs to be attached instead of default Toolbar..."

@sashadev-sky
Copy link
Member

@jywarren did I address this sufficiently in #393 to close this? I was going to do someting like L.LDITool.ActionName but it was so long I just stack gave them each L.ActionName and extended from L.EditAction

@jywarren jywarren unpinned this issue Dec 5, 2019
@sashadev-sky
Copy link
Member

This one is good to close now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants