Feature/mobile sidebar #726
Conversation
demonstrate use of `has/sniff` to configure the widgets based on the device
|
Looking very nice! At first glance, there seems to be an issue with the search widget.
|
|
either I missed checking in some css or mapTemplate.html or perhaps you have caching interfering. What device is that? |
viewer/js/viewer/_SidebarMixin.js
Outdated
| main: 'Sidebar', | ||
| location: modulesPath | ||
| }); | ||
| require(window.dojoConfig, [ |
There was a problem hiding this comment.
Hm, dynamic requires aren't supported when the build system scans modules. Why not just include the sidebar in the _SidebarMixin's dependencies?
There was a problem hiding this comment.
right. that's leftover from some original tests when the whole mixin and sidebar was not in the repo. will fix that.
There was a problem hiding this comment.
this was resolved in newest commit
There was a problem hiding this comment.
@roemhildtg I realize now why I had this originally as a dynamic require. We don't want the sidebar css file (and others) loaded unless the mobile sidebar is actually used. Files are loaded unnecessarily and the css can mess up the UI when using the default panes without the mobile sidebar like this:
I can address this with changes to the css but would rather it not load the files at all.
The build system and the sidebar are both important when considering mobile. I'll explore options. Let me know if you have any thoughts on this.
|
Oops, I think I had checked out the wrong commit |
|
We should problably implment the parentWidget function stuff. Otherwise the streetview and other widgets that use that won't work. if (this.parentWidget) {
if (this.parentWidget.toggleable) {
this.own(aspect.after(this.parentWidget, 'toggle', lang.hitch(this, function () {
this.onLayoutChange(this.parentWidget.open);
})));
}
this.own(aspect.after(this.parentWidget, 'resize', lang.hitch(this, 'resize')));
this.own(topic.subscribe(this.parentWidget.id + '/resize/resize', lang.hitch(this, 'resize')));
} |
|
Possibly or we need consider this from the opposite perspective. Widgets that have functionality dependent on a parentWidget (or 'floating') perhaps need to be re-worked. Widgets should work the same regardless of whether the type is a 'titlePane', 'contentPane', 'floating', or 'domNode' or wrapped in calcite maps. |
|
That's probably the better approach. I think one fundamental idea of all cmv widgets is their just "dijit widgets" and should be workable outside of cmv. And outside of cmv google streetview won't have a "parent widget" but it should still work. |
viewer/js/viewer/_LayoutMixin.js
Outdated
|
|
||
| loadConfig: function () { | ||
| this.detectTouchDevices(); | ||
| this.inherited(arguments); |
There was a problem hiding this comment.
I think this should be
return this.inherited(arguments);
viewer/js/viewer/_SidebarMixin.js
Outdated
| } | ||
| }); | ||
| this.mapDeferred.then(lang.hitch(this, '_createSidebar')); | ||
| } |
There was a problem hiding this comment.
I think this should also
return this.inherited(arguments);
There was a problem hiding this comment.
with this proposed change, there is an error calling this.mapDeferred.then(...
would changing the order of the mixins also be necessary?
There was a problem hiding this comment.
I think yes. Since mapDeferred doesn't exist until the map mixin is called, this mixin would need to load after the map mixin, meaning it should appear above the map mixin in the list (since the inheritance order is like an upside down stack). (top last, bottom first)
I think I said that right...does that make sense?
| @@ -73,7 +79,7 @@ define([ | |||
|
|
|||
| this.addTopics(); | |||
| this.addTitles(); | |||
| this.detectTouchDevices(); | |||
| this.setPhoneInfoWindow(); | |||
| this.initPanes(); | |||
|
|
|||
| this.mapDeferred.then(lang.hitch(this, 'createPanes')); | |||
There was a problem hiding this comment.
I just realized, this doesn't but should call
this.inherited(arguments);There was a problem hiding this comment.
It does on line 89. Is that not correct?
There was a problem hiding this comment.
Oh! yes, I just couldn't see it. I'm still figuring out how this review tool works...
viewer/js/viewer/_SidebarMixin.js
Outdated
| return declare(null, { | ||
|
|
||
| postConfig: function () { | ||
| this.inherited(arguments); |
…nfigurations. build-friendly alternatives welcome
|
For the build system, since the css would get put into a bundle, the users will either have to choose to use the sidebar or not. I think that's fine. What do @tmcgee and @DavidSpriggs think? |
|
@roemhildtg And so likewise, all css (and JS and html?) for all mixins, widgets would then be included in the build even if they are not used by the application and/or when conditionally excluded for certain devices. We have to leave that up to the individual builder to explicitly include the mixins and widgets they want in their custom build. Given that, is there anything more you would change in this PR? |
|
Correct, it will be up to the individual builder. It is possible to create multiple builds each with the widgets and components that the builder wants, but it gets a bit complicated and its something I plan to look into but for now am just bundling everything into one. Nothing more, will merge. |
Add a mobile-friendly sidebar layout adapted from @DavidSpriggs dojo-sidebar.
It is the default for phones.
Adaptable for use with other devices.
Can be disabled using 'sidebar: false` for original cmv behavior