Esri / application-boilerplate-3x-js

Starter application that simplifies the process of building templates for the ArcGIS.com template gallery.
https://developers.arcgis.com/javascript/
Apache License 2.0
90 stars 68 forks source link

Proposal for strong decoupling between boilerplate and app that uses it #86

Closed MikeTschudi closed 7 years ago

MikeTschudi commented 7 years ago

85

driskull commented 7 years ago

@MikeTschudi what about instead of runGroup, runMap. We just have start that gets passed an object that has properties of all the necessary things.

{
config:...,
view:...,
webscene:...,
webmap:...,
groupInfo:...,
groupItems:...
}
MikeTschudi commented 7 years ago

Also sounds like an improvement to me.

alaframboise commented 7 years ago

@driskull @MikeTschudi Just started working with this yesterday. Integrating your app code and references is definitely tricky.

Question: Where do we recommend people to put their app code? main.js? To configure the app and page, some code needs to be applied to the document framework e.g. CSS styles, title, description... and other code needs to be applied to the view properties e.g. padding before creation. Right now I'm just putting everything in main.js. I was looking for an app.js...

And I'm still trying to figure out the initialization (below). I guess I expected to:

  1. Create a boilerplate object().
  2. Get a webItem back.
  3. Do my app template stuff a) Set document HTML/CSS and element b) Load map or scene c) Other app specific stuff...
   require([
      "boilerplate",
      "dojo/text!boilerplate/settings.json",
      "application"
    ], function (
      Boilerplate,
      boilerplateSettings,
      applicationMain
    ) {

      // create my main application. Start placing your logic in the main.js file.
      var mainApp = new applicationMain();

      // create the template. This will take care of all the logic required for template applications. If unsuccessful, we're calling error on main. Otherwise, we call startup on main.
      new Boilerplate(JSON.parse(boilerplateSettings)).then(mainApp.init.bind(mainApp), mainApp.reportError.bind(mainApp));

    });
MikeTschudi commented 7 years ago

@alaframboise: The recommendation is to put the app code into main.js, with optional changes in index.html and a few other places. It probably won't be necessary to modify the code in the snippet that you showed.

In this pull request, you'll see three entry points into main.js:

  1. adjustConfig: uncomment this if you want to modify the configuration before the webmap/webscene/group gallery is created
  2. reportError: uncomment this if you want to change the error display that the boilerplate uses
  3. start: put your app code in here

Here are the places where I've made changes in my copy of the boilerplate to support a 3D 4.0 app:

  1. index.html
    • Added a link to my css/main.css after the other links
    • Added some divs after the other divs
    • Added script includes for my library routines after the other scripts
  2. config\config.json
    • Defined app defaults
  3. js\boilerplate\settings.json
    • Disabled fetches for webmaps and groups
    • Removed URL params not used in app
  4. js\application\main.js
    • Completely changed but for start() signature
  5. js\application\nls\resources et al.
    • Added strings
  6. resources\configurationPanel.js
    • Defined hosted app parameters

Plus I've added app-specific files in various places.

The boilerplate files that I modified don't tend to change, so I've found it no trouble to merge boilerplate updates several times a day into my app as we refine the decoupling design. For example, the boilerplate.js and init.js files have changed during the refinement, but because of the three-entry-point contract between the boilerplate and the main.js file, none of those changes required any changes to main.js for me--I just copied the files in.

driskull commented 7 years ago

@MikeTschudi the only remaining thing before merging is to separate the mapOrScene property like this:

{
webscene:...,
webmap:...,
}
MikeTschudi commented 7 years ago

Except that I prefer the polymorphism :-)

alaframboise commented 7 years ago

what about this

map: {
 id: "19faa71a3bf6468cae35b4fce9393a7d" 
 type: "webmap"
}
MikeTschudi commented 7 years ago

What we're debating is the signature of the start() function in main.js. It contains a single argument options with properties appropriate to what the boilerplate finds in the configuration (i.e., webmap app or webscene app or group gallery app?)

Two proposals:

options: {
  config,     // configuration assembled by boilerplate
  viewNode,   // dom node for main app display
  view,       // resolved MapView or SceneView created in init
  map,        // resolved WebMap or WebScene created in init
  groupInfo,  // ArcGIS item groupInfo for group
  groupItems  // list of groupItems in group
}
options: {
  config,     // configuration assembled by boilerplate
  viewNode,   // dom node for main app display
  view,       // resolved MapView or SceneView created in init
  webScene,   // resolved WebScene created in init
  webMap,     // resolved WebMap created in init
  groupInfo,  // ArcGIS item groupInfo for group
  groupItems  // list of groupItems in group
}

Because of possible ambiguity, the map property has also been proposed to be mapOrScene, but that's a bit awkward. To be parallel with view, maybe it should be web. :-)

kellyhutchins commented 7 years ago

I prefer option 2 because in my opinion it makes it more clear what value is expected (web map or web scene) vs the map option which I think could be confusing.

MikeTschudi commented 7 years ago

You broke the tie; I'll make the change!

kellyhutchins commented 7 years ago

Thanks @MikeTschudi for making these updates. I think your modifications will make it easier for template devs to keep in sync with any updates.

alaframboise commented 7 years ago

I just merged and this helps keep the app logic in one location. Thanks!