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

Menu Refactor #30

Merged
merged 13 commits into from
Apr 14, 2015
Merged

Menu Refactor #30

merged 13 commits into from
Apr 14, 2015

Conversation

alukach
Copy link
Member

@alukach alukach commented Apr 12, 2015

I've opened this up for review/discussion. It was built off of the feature/grunt_helpers branch (#29), so for sake of a cleaner diff I've set this to merge into that branch.

What's changed

In efforts to prepare for more menu options, I've broken all of the geo-tooling out of L.DNC.Menu and placed it in a L.DNC.GeoMenu object that inherits from L.DNC.Menu. The menu now builds itself based on it's own buildMenu() function.


},
L.DNC.GeoMenu = L.DNC.Menu.extend({
title: "Geoprocessing Tools",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this go into this.options? Probably...

@alukach
Copy link
Member Author

alukach commented Apr 12, 2015

Thinking more about this, I think this still has quite a ways to go.

There should probably be a single location (likely within src/js/menu) that constructs the menu. The menu will be composed of something like:

L.DNC.MenuBar = L.Class.extend({
    initialize : function( options ) {
        new L.DNC.Menu("Geoprocessing Tools", { 
          operations: [
            // The meat of the tooling can either be defined here or in a class that subclasses L.DNC.MenuOperartion
            new L.DNC.MenuOperation("Buffer", function() {
              // Do some buffering stuff
            }),
            new L.DNC.MenuOperation("Erase", new L.DNC.EraseOperation({}))
          ]
        });
        new L.DNC.Menu("Save Data", {
          operations: [
            // ...
          ]
        })

    }
});

or probably more like:

L.DNC.MenuBar = L.Class.extend({
    initialize : function( options ) {
        new L.DNC.Menu("Geoprocessing Tools", {})
          // These operations would all extend the same MenuOperation (and maybe GeoOperation) class
          .addOperation(new L.DNC.BufferOperation({}))
          .addOperation(new L.DNC.EraseOperation({}))
          .addOperation(new L.DNC.UnionOperation({}))
        ;

        new L.DNC.Menu("Save Data", {position: "right", icon: "fa-floppy-o"})  // Pass in menu options
          .addOperation(new L.DNC.DownloadJsonOperation({}))
          .addOperation(new L.DNC.DownloadShapeFileOperation({}))
          .addOperation(new L.DNC.CreateGistOperation({}))
        ;
    }
});

To reiterate from the last conversation, a menu operation should be aware of:

  • Types Supported (Points, Polygons, FeatureCollection, etc)
  • Number of args of supported
  • Menu Settings
    • Which menu it belongs in (can we support submenus?) (This will be controlled within the menu definition. Regarding submenu support, we should ensure that addOperation supports being given a L.DNC.Menu instance)
    • How it looks in menu (HTML)
  • Dialog Settings
    • Form components
    • Reorder Layers

@alukach
Copy link
Member Author

alukach commented Apr 12, 2015

Continuing with this, I think abstracting all Turf operations would be even more ideal:

L.DNC.MenuBar = L.Class.extend({
    initialize : function( options ) {
        new L.DNC.Menu("Geoprocessing Tools", {})
          .addOperation(new L.DNC.TurfOperation("buffer", {
            maxFeatures: 1
          }))
          .addOperation(new L.DNC.TurfOperation("erase", {
            minFeatures: 2,
            maxFeatures: 2,
            orderImportant: true
          }))
          .addOperation(new L.DNC.TurfOperation("union", {
            maxFeatures: 2,
            minFeatures: 2
          }))
        ;

        new L.DNC.Menu("Save Data", {position: "right", icon: "fa-floppy-o"})  // Pass in menu options
          .addOperation(new L.DNC.DownloadJsonOperation({}))
          .addOperation(new L.DNC.DownloadShapeFileOperation({}))
          .addOperation(new L.DNC.CreateGistOperation({}))
        ;
    }
});

Turf Operations Options

  • supportedFeatures
    • Types Supported (Points, Polygons, FeatureCollection, etc)
    • Array, default: all
  • minFeatures
    • Minimum number of layers required
    • Number, default: 1
  • maxFeatures
    • Maximum number of layers required
    • Number, default: 0
  • orderImportant
    • Whether order of layers is important. If true, enables reordering in dialog
    • Boolean, default: false

Experimental

  • iterateSafe
    • Whether it's save to iterate over a list of features (in no particular order) and apply operation on each feature. (It seems that while some operations (ex buffer) can only be applied to a single layer, there's no reason that we couldn't enable running the same buffer operation on any number of selected features.)
    • Boolean, default: false

Abstracting every turf operation may be lofty, but if we can do it correctly it could allow us to quickly support all operations with little code.

@mapsam
Copy link
Member

mapsam commented Apr 12, 2015

This is really cool - I'm super into the last example that you're showing. The structure seems really straightforward and easy to manage. It will be particularly helpful as we make decisions on what goes into each menu and need to change order/location quickly. Doesn't involve going to individual files for each of the processes.


initialize: function ( title, options ) {
L.DNC.Operation.prototype.initialize.call(this, title, options);
},
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to override method to simply call parent method.

@mapsam
Copy link
Member

mapsam commented Apr 13, 2015

I wonder if we could also add an "info" parameter that allows us to write a paragraph about the specific type of operation. I.e. when you hover on buffer for long enough the alt comes up with the options and what a buffer actually is. Could just be passed as an HTML string for now.

@alukach
Copy link
Member Author

alukach commented Apr 14, 2015

I wonder if we could also add an "info" parameter that allows us to write a paragraph about the specific type of operation.

@svmatthews That's funny, I was thinking about the exact same thing today. I think I'm going to take up tooltips as the next feature.

For now, I think that this is complete. I've met the minimum buffer, union, and erase functionality that was previously wired together. It should be a breeze to add other Turf operations. With that said, there's still some work ahead. Notably, figuring out a way to create a dialog based on the TurfOperation options and to wire that up when needed (the buffer size use-case is a good place to start).

I also went ahead and moved that event handler into the FileReader logic that you mentioned in #31. It wasn't as big of a deal as I originally thought.

This stuff probably is worthy of creating some tests, although I haven't gotten to that just yet.

@mapsam
Copy link
Member

mapsam commented Apr 14, 2015

Oh man this is amazing. This whole pub/sub system seems really powerful. I'm going to work around on this and will merge it asap, then rebase for #33 to have notifications listen for publishers all in one file, rather than having them spread all around.

mapsam added a commit that referenced this pull request Apr 14, 2015
@mapsam mapsam merged commit 0aab595 into feature/grunt_helpers Apr 14, 2015
@mapsam mapsam deleted the refactor/menus branch April 14, 2015 03:42
@mapsam
Copy link
Member

mapsam commented Apr 14, 2015

💥 shakalaka!

@mapsam
Copy link
Member

mapsam commented Apr 14, 2015

When these refactors happen, I'm going to start making wiki pages so we can document once they are settled. Here's the placeholder for the menu architecture. https://github.com/cugos/drop-n-chop/wiki/Menu-Items

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

Successfully merging this pull request may close these issues.

2 participants