Use inherited mixins #666

green3g · 2017-02-01T17:00:52Z

make mixins less dependent on each other by implementing common methods and this.inherited
- loadConfig - a method that can load and modify the config object before the app begins loading
- postConfig - a method that can initialize app variables and perform any necessary operations after the config is loaded and processed, but before startup happens
- startup - a method that starts the app layout, map, widgets, etc

…eds for async ops

…eferred

- as a result, order is not important with map/webmap mixins

green3g · 2017-02-06T15:53:49Z

@tmcgee @DavidSpriggs Can this be merged?

tmcgee · 2017-02-06T19:23:21Z

@roemhildtg As you know for our conversations, I like this. I have not yet had time to run through it locally and consider how it relates to other mixins I have created/used and am considering. There might be a tweak or 2 as an outcome but I think it is good direction and very close to done.

green3g · 2017-02-06T19:39:54Z

Sounds good, I just wanted to see where we're at. I have some additional mixin ideas and would like to have this merged before I started exploring those.

tmcgee

3 places where promise/all is included but not used.
1 change to the final call to createWidgets

tmcgee · 2017-02-07T23:24:23Z

viewer/js/viewer/_LayoutMixin.js

@@ -11,6 +11,8 @@ define([
    'dojo/dom-class',
    'dojo/dom-geometry',
    'dojo/sniff',
+    'dojo/Deferred',
+    'dojo/promise/all',


promiseAll is not used in this Mixin

tmcgee · 2017-02-07T23:25:44Z

viewer/js/viewer/_MapMixin.js

@@ -5,6 +5,7 @@ define([
    'dojo/dom',
    'dojo/_base/array',
    'dojo/Deferred',
+    'dojo/promise/all',


promiseAll is not used in this Mixin

One more then you can take that well-deserved day-off. 😉

tmcgee · 2017-02-07T23:26:27Z

viewer/js/viewer/_WebMapMixin.js

@@ -2,6 +2,8 @@ define([
    'dojo/_base/declare',
    'dojo/_base/lang',
    'dojo/_base/array',
+    'dojo/promise/all',


promiseAll is not used in this Mixin

tmcgee · 2017-02-07T23:29:14Z

viewer/js/viewer/_WidgetsMixin.js

+            if (this.layoutDeferred) {
+                promiseAll([this.mapDeferred, this.layoutDeferred])
+                    .then(lang.hitch(this,
+                        'createWidgets', ['titlePane', 'contentPane', 'floating', 'domNode', 'invisible', 'layout']


this final call to createWidgets should pass null or this.widgetTypes. This allows the dev to not have to override this entire function if they add any custom widget types.

No. Each widget can only be loaded once based on the key in the widgets object (or the id property, if provided). The code in the createWidget method checks to see if a widget with the same key/id has already been loaded.

Yup, I saw that after I commented. (then I deleted my comment but you were too quick 😄 )

so now it looks like I was responding only to the voices in my head! 😮

tmcgee · 2017-02-08T00:40:29Z

After my review, I tested these changes with my WAB Widgets mixin, Calcite Maps mixin and my work-in-progress mixins for version 4.x of the Esri JavaScript API. As I expected, this required a few minor changes in the 4.x mixins. Other than that, it seemed to work well without a hitch. With this change, I think I might be able to remove some of the override functions in my mixins. nice work!

…cmv-app into HEAD

tmcgee · 2017-02-11T18:13:29Z

viewer/js/viewer/_WidgetsMixin.js

+            }
+            if (this.layoutDeferred) {
+                promiseAll([this.mapDeferred, this.layoutDeferred])
+                    .then(lang.hitch(this, 'createWidgets'));


you have to pass an argument (null or this.widgetTypes). No widgets are created when you don't.

I thought that this.widgetTypes is used automatically here

widgetTypes = widgetTypes || this.widgetTypes;

The link doesn't work but its line 75 of _WidgetsMixin

it does't use this.widgetTypes when it is called this way using lang.hitch. I tried that before posting my original suggestion.

Ahh, I see. since lang.hitch is passing the arguments from promiseAll.

tmcgee · 2017-02-11T21:17:16Z

viewer/js/viewer/_WidgetsMixin.js

            }
        },

        createWidgets: function (widgetTypes) {
+            debugger;


I hate it when I leave these in code! ;)

I swear I removed that...ahh, I must've staged it before I removed that line. Its my day off, I need a break :)

green3g added 7 commits January 16, 2017 14:39

implement an 'init' and 'startup' method in mixins and utilize deferr…

bcd493e

…eds for async ops

async loading of loadConfig and preStartup methods

7cb59c3

use createMap method to allow mixins to modify map before resolving d…

6f319d7

…eferred

override createMap method if mixin returns a deferred

76ef0db

- as a result, order is not important with map/webmap mixins

remove console.log

705ecb6

add documentation and fix typo

44caebf

Merge branch 'develop' into develop

50cc823

DavidSpriggs added the in progress label Feb 1, 2017

fix lint errors

f3cf778

green3g force-pushed the use-inherited-mixins branch from 9043663 to f3cf778 Compare February 1, 2017 17:22

call createPanes when mapDeferred resolves

5c6b537

green3g mentioned this pull request Feb 1, 2017

Document mixin 'common' methods cmv/cmv-docs#21

Closed

green3g added the proposal label Feb 1, 2017

green3g added 2 commits February 1, 2017 15:30

rename preStartup to postConfig for clarity

5c65783

reset viewer.js to develop

9300ebb

green3g force-pushed the use-inherited-mixins branch from d997a06 to 9300ebb Compare February 1, 2017 21:43

Merge branch 'develop' into use-inherited-mixins

92dad0a

tmcgee self-assigned this Feb 7, 2017

tmcgee added this to the v2.0.0-beta.1 milestone Feb 7, 2017

tmcgee requested changes Feb 8, 2017

View reviewed changes

green3g added 2 commits February 11, 2017 11:56

Cleanup cmv#666

94baf8b

Merge branch 'use-inherited-mixins' of https://github.com/roemhildtg/…

5268fd3

…cmv-app into HEAD

tmcgee reviewed Feb 11, 2017

View reviewed changes

pass null to create 'all' widgets

ce28af2

green3g force-pushed the use-inherited-mixins branch from 2a7f35a to ce28af2 Compare February 11, 2017 21:20

remove extra promiseAll reference

dd2f8dc

tmcgee approved these changes Feb 11, 2017

View reviewed changes

tmcgee merged commit 1feae77 into cmv:develop Feb 11, 2017

DavidSpriggs removed the in progress label Feb 11, 2017

green3g deleted the use-inherited-mixins branch February 11, 2017 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use inherited mixins #666

Use inherited mixins #666

green3g commented Feb 1, 2017 •

edited

Loading

green3g commented Feb 6, 2017

tmcgee commented Feb 6, 2017

green3g commented Feb 6, 2017

tmcgee left a comment

tmcgee Feb 7, 2017

tmcgee Feb 7, 2017

tmcgee Feb 11, 2017

tmcgee Feb 7, 2017

tmcgee Feb 7, 2017

tmcgee Feb 11, 2017

green3g Feb 11, 2017

tmcgee Feb 11, 2017

tmcgee commented Feb 8, 2017

tmcgee Feb 11, 2017

green3g Feb 11, 2017

green3g Feb 11, 2017

tmcgee Feb 11, 2017

green3g Feb 11, 2017

tmcgee Feb 11, 2017

green3g Feb 11, 2017

Use inherited mixins #666

Use inherited mixins #666

Conversation

green3g commented Feb 1, 2017 • edited Loading

green3g commented Feb 6, 2017

tmcgee commented Feb 6, 2017

green3g commented Feb 6, 2017

tmcgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmcgee commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

green3g commented Feb 1, 2017 •

edited

Loading