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

Added scalebar option to ol.control.scaleline #9013

Merged
merged 3 commits into from Jan 23, 2019

Conversation

weskamm
Copy link
Contributor

@weskamm weskamm commented Nov 28, 2018

This adds an option to show scalebars instead of a scaleline in the ScaleLine control.

scalebar

Example and API doc have been adjusted.

examples/scale-line.js Outdated Show resolved Hide resolved
examples/scale-line.js Outdated Show resolved Hide resolved
examples/scale-line.js Outdated Show resolved Hide resolved
@tschaub
Copy link
Member

tschaub commented Nov 28, 2018

Thanks for the contribution @weskamm! My suggestions above are mostly about shorter option names.

I'm curious what you and others think about making the default style look a bit more like the current scaleline style.

linebar

Also wondering if it is possible to keep the bar shorter for longer.

Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @weskamm. I share @tschaub's concerns about the length of the scale bar. Regarding the style, I also agree that it should look a bit more like the style of the other controls. However, we could also think this the other way around and make the default theme for other controls look more black/white/grey.

@ThomasG77
Copy link
Contributor

ThomasG77 commented Nov 28, 2018

I'm not sure to understand the purpose of this PR as "ol-ext" has already an extension for this e.g http://viglino.github.io/ol-ext/examples/canvas/map.canvas.control.html
Should it be maintain in the core? I misunderstood?

@tschaub
Copy link
Member

tschaub commented Nov 28, 2018

However, we could also think this the other way around and make the default theme for other controls look more black/white/grey.

I like that idea. Probably best for a separate PR.

Copy link
Member

@marcjansen marcjansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A very nice PR, and a nice set of comments / suggestions.

I am 👍 on merging, once the remarks are adressed.

Thx.

@weskamm
Copy link
Contributor Author

weskamm commented Nov 29, 2018

Thanks for the reviews!
I adressed the issues mentioned by @tschaub (renamed props, fixed example, localization).

Regarding the length of the scalebar: I use a non-default minWidth in the example, because the default width of 64 pixels is a bit small for a scalebar (but ok for scaleline). Thats why the sizes differ. I just reduced the width in the example to get more similar results

Regarding the style: I would like to keep the current black and white look, as this is the way a scalebar typically looks like.

Copy link
Member

@tschaub tschaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the great contribution @weskamm.

@tschaub tschaub mentioned this pull request Aug 7, 2022
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.

None yet

5 participants