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

Use minus character instead of hyphen in the zoom control #5501

Merged
merged 3 commits into from
Jun 4, 2017

Conversation

damianmoore
Copy link
Contributor

I noticed that the minus sign in the zoom control was probably just a normal hyphen as it was narrow. The proper minus character is the exact same with as the plus character and has consistent vertical alignment. More info here: https://en.wikipedia.org/wiki/Plus_and_minus_signs#Character_codes

After replacing the hyphen with the minus from Wikipedia I checked a zoomed in screenshot of it in GIMP. The minus sign was wider than the plus because the font size had been enlarged - presumably because someone thought it looked too small in comparison. Now they can be the same font size which saves a bit of CSS too.

@perliedman
Copy link
Member

The same change was rejected in #2496, so even though this sounds like a good idea, it would need to be thoroughly tested on different OS/browser combinations.

@damianmoore
Copy link
Contributor Author

Strange that I didn't see the closed pull request - apologies. Interesting reading that it didn't display properly on Chrome/OS X. I'll change it to the '\u2212' character to be sure and then do as much testing as I can. The bit where mourner mentions vertical alignment should be fixed already in my pull request as I changed the CSS so they both use the same styles.

@nathancahill
Copy link
Member

I signed up for an open source BrowserStack account to test this on different OS/browsers. From what I could see in the free tier, all seem to support the u2212 character in the Lucida Console, Monaco, monospace bold font stack. I'll check again once I have access to all OS/browser combinations.

@nathancahill
Copy link
Member

Here's the HTML I'm testing with:

<span style="font: bold 18px 'Lucida Console', Monaco, monospace;">&#x2212;</span>

@damianmoore
Copy link
Contributor Author

@nathancahill I've switched to your HTML escaped suggestion. Still works for me. Thanks for testing on BrowserStack. I should be able to test a few devices myself tomorrow.

@nathancahill
Copy link
Member

Sorry, wasn't a suggestion to use the escaped character in JS, that was just for testing in HTML. Stick with '\u2212' in JS.

@nathancahill
Copy link
Member

nathancahill commented May 19, 2017

Tested on all of Leaflet's supported browsers on BrowserStack, u2212 works fine.

@damianmoore
Copy link
Contributor Author

Here are a few different browser tests I've done if it helps. Versions are the most recent unless otherwise stated. Not sure why Chromium/Chrome buttons are bigger and without box shadows but I don't think it matters.

leaflet_browser_testing

Copy link
Member

@perliedman perliedman 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 thorough testing!

@perliedman perliedman merged commit 60e1965 into Leaflet:master Jun 4, 2017
@jjimenezshaw jjimenezshaw mentioned this pull request Jun 30, 2017
6 tasks
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