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

Add ability to nest the viewer in a specific DOM element by ID #468

Conversation

JoelDCarter
Copy link

This pull request adds support in the viewer's controller for specifying the DOM element that the viewer's main BorderContainer should be added to via an optional viewer.js config file setting.

This helps with custom layouts where it's undesirable to append the BorderContainer to the document body. The current behavior is maintained if new 'container.id' setting is not set.

… the DOM node/element that the viewer's BorderContainer should be added to
@btfou
Copy link
Contributor

btfou commented Oct 28, 2015

This seems reasonable to me. Thanks for adding commented config info to viewer.js as well.

@tmcgee ?

@JoelDCarter
Copy link
Author

@btfou @tmcgee No, problem. Thanks for considering my PR

@tmcgee
Copy link
Member

tmcgee commented Oct 28, 2015

This definitely seems reasonable. Sorry that my overhaul of the project files caused a conflict. Fortunately that is easily resolved.

I've been thinking about this PR. In addition to the hard-coded document.body for BorderContainer, we also have other hard-coded references like mapCenter for the map's target ID. What do you think of flipping around the proposed container object in the config and structuring it something like:

ui: {
    container: 'cmv',
    map: 'myMapID'
},

Perhaps ui is not the correct object name but flipping the structure offers additional flexibility and fits some other UI concepts I am working on.

Thoughts?

@btfou
Copy link
Contributor

btfou commented Oct 29, 2015

I think this naturally leads to loading a layout Class as a templated widget or otherwise. Requires pretty big changes to the way Controller works, how widgets are loaded, and so on. That discussion needs to continue.

As far this PR, it's a good addition with a real use case. I propose layout as the term.

layout: {
  /* options */
}

@tmcgee
Copy link
Member

tmcgee commented Oct 29, 2015

layout works for me.

@tmcgee
Copy link
Member

tmcgee commented Oct 29, 2015

It isn't as big a change when we get Mixins going for Controller. That's my next PR. ;)

@JoelDCarter
Copy link
Author

The config structure that I submitted was

container: {
  id: 'borderContainerId'
}

The reason for this was that I wasn't sure if you may want to add other settings for the main container, ex. type, etc.

Would it be good to keep that structure and move it under a new layout section that @btfou proposed and adding @tmcgee's map settings section making it:

layout: {
  container: {
    id: 'borderContainerId'
  },
  map: {
    id: 'mapId'
  }
}

Or is that unnecessarily complex?

@tmcgee I could pull in the latest develop branch and make some of these changes on Friday if it would help. Just let me know.

Thanks!
Joel

@tmcgee
Copy link
Member

tmcgee commented Oct 29, 2015

@JoelDCarter Can you provide 1 or 2 example use cases for additional items within container/map/etc beyond the id? I could certainly support that notion but would like to hear more examples of how it might be used. Thanks for the contribution and discussion.

@DavidSpriggs
Copy link
Member

The core concept is good and the added flexibility is nice. Let's have a look at the use cases. I think we can make this work at some level. As both @btfou and @tmcgee pointed out, this has other implications in our long term plan but also leeds us in the right direction (Different themes etc.). Thanks @JoelDCarter for the PR and discussion!

@tmcgee
Copy link
Member

tmcgee commented Oct 29, 2015

I have add the optional layout object to the config in PR #475. I did not add the additional level of having an object with an id property. I am more than willing to change that if there are use cases to support it.

In that PR, I refactored the Controller into separate mixins so the relevant line is here in the new _LayoutMixin.js

Feedback and discussion is welcome and encouraged! :)

@DavidSpriggs
Copy link
Member

@JoelDCarter Thanks for this PR and discussion. There is a conflict with this PR now so I'm going to close it but the good news is that this concept made it into core. Thanks for the great idea. Keep them coming.

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

Successfully merging this pull request may close these issues.

4 participants