Leaflet.DistortableImage icon indicating copy to clipboard operation
Leaflet.DistortableImage copied to clipboard

abstract tools UI from tools functions to allow for alternative UIs

Open jywarren opened this issue 6 years ago • 17 comments

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; an alternative to the above syntax might be to pass in the toolbar as a parameter, like:

var altToolsUI = new SomeOtherToolsUI();

img = new L.DistortableImageOverlay(
        'example.jpg', {
          tools: altToolsUI,
          corners: [
            new L.latLng(51.52,-0.10),
            new L.latLng(51.52,-0.14),
            new L.latLng(51.50,-0.10),
            new L.latLng(51.50,-0.14)
          ]
        }
).addTo(map);

...with the current toolbar as the default if it's not set.

Or, we could do:

img = new L.DistortableImageOverlay(
        'example.jpg', {
          corners: [
            new L.latLng(51.52,-0.10),
            new L.latLng(51.52,-0.14),
            new L.latLng(51.50,-0.10),
            new L.latLng(51.50,-0.14)
          ]
        }
)
  .addTools(altToolsUI)
  .addTo(map);

Then, the actual functions like Scale and Rotate and others would remain in this file:

https://github.com/publiclab/Leaflet.DistortableImage/blob/0bce3c41ae2d598221f873b3d3d2866541a3e03d/src/edit/DistortableImage.Edit.js#L97

while the UI would bind to those functions in this default toolbar or another UI:

https://github.com/publiclab/Leaflet.DistortableImage/blob/0bce3c41ae2d598221f873b3d3d2866541a3e03d/src/edit/DistortableImage.EditToolbar.js#L73-L93

Ideas welcome here!

Relates to #112 which allows for separate Rotate/Scale tools.

jywarren avatar Feb 18 '19 16:02 jywarren

An example UI could just be a different implementation of Leaflet.Toolbar that uses the Control display instead of a Popup:

image

https://github.com/Leaflet/Leaflet.toolbar

jywarren avatar Feb 18 '19 17:02 jywarren

I already pitched this idea to @jywarren, it implements a mixture of what he has mentioned above, but @rexagod let me know what your thoughts are on this too!. I'm excited to work on this with you!!

@jywarren I have really cleared up the implementation details at this point for the modal menu so maybe we can weigh pros and cons?

I used the Google Maps JavaScript API and the Material UI Kit library as inspiration for this implementation plan:

  1. we can provide a bare minimum toolbar as the initial default (with option to suppress as implemented by rexagod)

  2. The toolbar could be a collapsible module that comes fully functional (as a class that handles its own state for switching tab panes, etc) and styled out of the box. We can define some very basic styling options on it but the main implementation will be to provide "feature types" options that the user can initialize it with or add on later. The feature types would be sensible collections of stand-alone modules such as "exports", "uploads" "image adjustments", "image manipulation", etc. also functional out of the box). Each collection could get its own content tab.

  3. Once that works, we can create even smaller collections of "element types" that the user can select out of the features if they don't want all of them. I think that most design elements travel in some sort of pair (ie. Hue, Saturation & Lightness, Rotate & Scale, Sharpen & Blur)

@jywarren Also I am seeing now my idea is similar to yours but backwards. I would not mind starting bottom-up instead and isolating all of the modules first.

Anyway I definitely want to turn my attention to the export system now since it seems more priority so I am happy to just leave this here as an idea for now and come back to it

sashadev-sky avatar Feb 19 '19 12:02 sashadev-sky

OK let's continue discussing -- but yeah, i think if we treat the current Leaflet.Toolbar lib as the "default" but allow it to be overridden by another more complex tools UI, perhaps as you describe, that'd be cool. Note that the React wrapper overrode the Toolbar, so could be a good example to work from: #105

jywarren avatar Feb 19 '19 16:02 jywarren

screenshot from 2019-02-22 03-44-17

@jywarren Something like this?

rexagod avatar Feb 21 '19 22:02 rexagod

Oh that looks really nice!

jywarren avatar Feb 21 '19 22:02 jywarren

@rexagod @jywarren what are your thoughts on this outline? @rexagod this is made on the mapknitter UI so the map looks different. (I don't think I scaled it very well image the images added to be smaller than you would because everything else is smaller) mockup_edited-1

Toolkit would be the toggleable dropdown, login and resources, etc would go under information tab. Maybe the arrow on the right would turn down on click and give a little strip of extra tools, or, it would be a toolbar menu that would move right to present new icons. Not committed to the icon choices.

Legend can also live in the toolbar information if we want it to.

sashadev-sky avatar Feb 25 '19 16:02 sashadev-sky

wow this is cool. What would the different items be in the right-side column? info, images, ... ?

Can we show a few images selected in the upcoming "multiple image selection" and show how they'd appear in the sidebar?

Great work!

jywarren avatar Feb 27 '19 23:02 jywarren

Yeah this is big enough that perhaps it belongs in its own library built on top of this one... like, Leaflet.DistortableImageToolbar or something??? Just thinking though in terms of keeping this library more universal -- and here we're talking about some big changes! What do you think?

jywarren avatar Feb 27 '19 23:02 jywarren

Also just to keep our vocabulary clear here, let's call anything within a toolbar a tool but the whole popup/control style thing is a toolbar. This issue is about abstracting tools from tool UIs (like the toolbar implementation) so that we can build totally new UIs or toolbars that can still trigger the same actions on the distortable images. I hope that makes some sense!!!

jywarren avatar Feb 28 '19 19:02 jywarren

And to further clarify -- after my comment here: https://github.com/publiclab/Leaflet.DistortableImage/pull/147#pullrequestreview-209255378

Why don't we open a new separate issue for the UI for handling multiple images. Then we can move @sashadev-sky 's comment (https://github.com/publiclab/Leaflet.DistortableImage/issues/140#issuecomment-467079304) there, and focus on that discussion separately from the work on individual tools and toolbars that attach to a single image, which is a bit of a different case.

Thanks for bearing with me as I parse through this all!

jywarren avatar Feb 28 '19 19:02 jywarren

@jywarren I didn't fill out the right side because I wanted to get your initial opinion, but what I was going for would be

  1. info: maybe include login, resources, there.
  2. Images for uploads.
  3. Then transform editing: anything that has to do with size and distorting the image, moving the image, etc.
  4. Next to that photo editing features: brightness, opacity, etc.
  5. Last is exports.

Okay that will be my next step then. Get the images to be selected and then create a single toolbar view with the selected images. It is some very big changes! That could be step 2 perhaps after getting the first view working? It's easier to get it integrated with the current library I think and then removing it into its own. I am not sure though, what are your thoughts?

Noted for toolbars vs tools. Definitely makes sense.

We can definitely open a separate issue to address it! Do you think first the basic functionality should be complete? For me this is an intuitive next step: functionality issue -> UI issue

sashadev-sky avatar Mar 01 '19 08:03 sashadev-sky

Let's move to a new issue UI for handling multiple images if that's OK? Can you copy in all these comments because it's starting to overflow the original issue. Then we can "resolve" and minimize them here. Thanks!

Get the images to be selected and then create a single toolbar view with the selected images. It is some very big changes!

Agreed, and i think we can do the most minimal implementation of this to start with. Like, we could even just have multiple selected images be highlighted first, no UI of the menu just yet.

That could be step 2 perhaps after getting the first view working? It's easier to get it integrated with the current library I think and then removing it into its own. I am not sure though, what are your thoughts?

I think that's true and maybe we can do the absolute minimum interface for multiple image selection in this lib, before allowing an external UI to be attached to them (this could be in its own library maybe). Thanks!!!

jywarren avatar Mar 04 '19 23:03 jywarren

@jywarren Ok I am going to add this as another todo on the multiple image select PR I have open. That way when I finish implementing functionality I will remember to open the issue. Is that ok?

sashadev-sky avatar Mar 12 '19 03:03 sashadev-sky

I think #147 completes some significant portion of this, which is great. I do want to highlight one part that isn't addressed and could be broken out into it's own issue: the idea of making it possible for the toolbar UI that contains the current rotate, distort, lock, etc tools, to be turned off and/or replaced with another toolbar-like UI entirely. We have something almost there - suppressToolbar i think it's called? But this is a subtle difference - actually making the current toolbar just the default, and one which could be overridden by another UI that is triggered to appear upon clicking the image.

Still, this isn't really that high priority of an issue, it is just an idea to help organize the code better. The more important parts are in #147 -- thank you!!!

jywarren avatar Mar 14 '19 23:03 jywarren

@jywarren I see what you are saying. Interesting! @rexagod what are your thoughts since you worked on #147

sashadev-sky avatar Mar 16 '19 03:03 sashadev-sky

@jywarren @sashadev-sky I think we can migrate this over to #170 now. Please have a look!

Thanks!

rexagod avatar Mar 18 '19 18:03 rexagod

wasn't sure where to put this but just saw this on a page on the Mapbox site and thought it was nice for future UI inspiration:

dataset-editor

On the right it looks like there is a toolbar for the map as a whole, then theres an exporter UI that also implements a search functionality, then on the left you can see a separate toolbar for an individual section

sashadev-sky avatar Mar 28 '19 04:03 sashadev-sky