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

Viewer mixins #853

Merged
merged 8 commits into from
Jun 11, 2013
Merged

Viewer mixins #853

merged 8 commits into from
Jun 11, 2013

Conversation

mramato
Copy link
Contributor

@mramato mramato commented Jun 10, 2013

Note: This is a pull into the viewer branch.

We can do final cleanup in the viewer branch, but open a separate pull so we can make sure we all agree on the approach.

@shunter Take a look at this and merge if you're happy.
@emackey Your comments are also welcome (obviously so are everyone else's).

@mramato
Copy link
Contributor Author

mramato commented Jun 10, 2013

I ended up submitting some other cleanup to this branch as well, so the sooner we can agree and get this back into the viewer branch, the better.

@emackey
Copy link
Contributor

emackey commented Jun 10, 2013

I like the mixins 👍 mostly because it makes the code look as if the widget had the functionality built-in, while still providing an option for the same widget without the functionality.

I asked this in person, but are we sure all of this widget's functionality shouldn't just be a mixin for the base CesiumWidget? Probably a lot of lost work if we change that plan now, but are we going to decide to do that down the road?

@shunter
Copy link
Contributor

shunter commented Jun 11, 2013

The Viewer widget doesn't have much functionality beyond composing the sub-widgets. One issue with making the sub-widgets mixins too is that we'd need some kind of notification system, e.g. the DynamicObject mixin depends on the homeButton widget, if it exists.

I think this is a good start. We can tweak later if we need to. Merging.

shunter added a commit that referenced this pull request Jun 11, 2013
@shunter shunter merged commit 86861ce into viewer Jun 11, 2013
@shunter shunter deleted the viewer-mixins branch June 11, 2013 15:36
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.

3 participants