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

Make GeoExt.Overview inherit from Ext.Component #292

Merged
merged 1 commit into from
Nov 5, 2014

Conversation

bentrm
Copy link
Member

@bentrm bentrm commented Nov 5, 2014

Just realized after the fact that letting the overview component inherit from Ext.container.Container would invite to add child items to it which will not work (and probablly shouldn't anyway). Also, I think it's cleaner to go for Ext.Component as it will also reduce the footprint of the overview. Sorry, for introducing this change after the merge of #288.

@chrismayer
Copy link
Contributor

Hi @bentrm

you are right, extending from Ext.Component would be more straight forward due to the arguments you wrote in your comment.
But now the name GeoExt.Overview seems to be a bit misleading. Something like GeoExt.OverviewMap seems to be more intuitive (at least for me). What do you think?

Seems that we all missed this or does someone else arguments against this especially @marcjansen since he has been strongly involved in #288 ?

@bentrm
Copy link
Member Author

bentrm commented Nov 5, 2014

I agree. If @marcjansen is with us I will change the alias to gx_overviewmapas well.

@marcjansen
Copy link
Member

👍 for gx_overviewmap and GeoExt.OverviewMap and for changing the inheritance chain.

We're in between releases, so no harm done IMO

@bentrm
Copy link
Member Author

bentrm commented Nov 5, 2014

This should be good to go now.

@marcjansen
Copy link
Member

Thanks again, @bentrm.

marcjansen added a commit that referenced this pull request Nov 5, 2014
Make GeoExt.Overview inherit from Ext.Component
@marcjansen marcjansen merged commit 71b737d into geoext:master Nov 5, 2014
@bentrm bentrm deleted the overview branch November 13, 2014 08:38
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