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 and use a base class for mappanel sliders. #204

Merged
merged 1 commit into from Oct 29, 2013

Conversation

Projects
None yet
2 participants
@marcjansen
Member

marcjansen commented Oct 29, 2013

This reduces the amount of copied code and reuses the methods from the base class.

All tests pass in IE8, Chrome 30 and Firefox 24, The examples work as expected in these browsers.

Please review.

PS: The Zoom slider example still shows some glitches when the slider outside of the mappanel is being used in a quick manor. AFAICT this is not related to the changes in this PR, as it can e.g. be reproduced in the online example. I opened a separate issue for this behaviour: #205.

Ext.define('GeoExt.slider.MapPanelItem', {
extend: 'Ext.slider.Single',
requires: [
'GeoExt.Version'

This comment has been minimized.

@bartvde

bartvde Oct 29, 2013

Member

maybe I have missed previous discussion on this but I wonder why we need this

@bartvde

bartvde Oct 29, 2013

Member

maybe I have missed previous discussion on this but I wonder why we need this

This comment has been minimized.

@marcjansen

marcjansen Oct 29, 2013

Member

This enables us to have GeoExt.version and related properties from GeoExt.Version print sane values.

This was introduced in #201, see there for discussion. Since MapPanelItem might be the only class required by the autoloading mechanism, we need to ensure that GeoExt.Version is loaded.

I am 100% open to a more automatic and non verbose way of always having GeoExt.Version in the build (or being fetched lazily). PR more than welcome 😉

@marcjansen

marcjansen Oct 29, 2013

Member

This enables us to have GeoExt.version and related properties from GeoExt.Version print sane values.

This was introduced in #201, see there for discussion. Since MapPanelItem might be the only class required by the autoloading mechanism, we need to ensure that GeoExt.Version is loaded.

I am 100% open to a more automatic and non verbose way of always having GeoExt.Version in the build (or being fetched lazily). PR more than welcome 😉

This comment has been minimized.

@bartvde

bartvde Oct 29, 2013

Member

okay trying to figure out how Ext actually does it with src/version/Version.js

@bartvde

bartvde Oct 29, 2013

Member

okay trying to figure out how Ext actually does it with src/version/Version.js

@bartvde

This comment has been minimized.

Show comment
Hide comment
@bartvde

bartvde Oct 29, 2013

Member

LGTM @marcjansen put in two minor comments

Member

bartvde commented Oct 29, 2013

LGTM @marcjansen put in two minor comments

Add and use a base class for mappanel sliders.
This reduces the amount of copied code and reuses the methods from the
base class.

marcjansen added a commit that referenced this pull request Oct 29, 2013

Merge pull request #204 from marcjansen/refactor-slider
Add and use a base class for mappanel sliders. (r=@bartvde)

@marcjansen marcjansen merged commit 9e3459d into geoext:master Oct 29, 2013

@marcjansen marcjansen deleted the marcjansen:refactor-slider branch Oct 29, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment