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

Including spaces between Magnitude and Unit in Scale Control Formatting #12644

Merged
merged 2 commits into from
Apr 12, 2023

Conversation

kathirgounder
Copy link
Contributor

@kathirgounder kathirgounder commented Apr 6, 2023

Tackling Scale Control Formatting Consistency Issue #12620. Decided that including spaces between magnitude and unit in the Scale Control for all units would be the best practice, this will also ensure consistency between the legacySetScale (includes spaces) function and _setScale (currently does not include spaces).

Before:
Screenshot 2023-04-07 at 6 37 15 PM

After:
Screenshot 2023-04-07 at 6 38 08 PM

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Adding Spaces between Magnitude and Unit in the Scale Control for all units</changelog>

@CLAassistant
Copy link

CLAassistant commented Apr 6, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

If we're removing the space, for consistency, we should also remove it in legacySetScale (that's used when Intl.NumberFormat isn't available).

However, I'm wondering if it would be better to change it the other way around — looking at the formatting docs, we could replace "narrow" in options with "short" and that would add the space. And the non-breaking part could be enforced in CSS (white-space: nowrap). Not sure. What do you think?

@kathirgounder
Copy link
Contributor Author

kathirgounder commented Apr 8, 2023

If we're removing the space, for consistency, we should also remove it in legacySetScale (that's used when Intl.NumberFormat isn't available).

However, I'm wondering if it would be better to change it the other way around — looking at the formatting docs, we could replace "narrow" in options with "short" and that would add the space. And the non-breaking part could be enforced in CSS (white-space: nowrap). Not sure. What do you think?

Hey @mourner !! Thanks for the comment (also big fan of your leaflet work, trying to be like you one day!) I think I agree with changing it to the other way around, I like the space between the magnitude and unit ! I will try to draft something up

Edit: Seems like there's actually a discrepancy between the legacySetScale and _setscale as the legacysetscale actually includes a non-breaking space (it doesn't use intl.numberFormat) between the magnitude and the unit for all types of units not just nautical, so I like adding the space in _setScale even more now

Screenshot 2023-04-07 at 6 08 31 PM

@kathirgounder kathirgounder changed the title Removed Non-Breaking Space in hardcoded nautical unit formatting Including spaces in hardcoded nautical unit formatting Apr 8, 2023
@kathirgounder kathirgounder changed the title Including spaces in hardcoded nautical unit formatting Including spaces between Magnitude and Unit in Scale Control Formatting Apr 8, 2023
@mourner
Copy link
Member

mourner commented Apr 11, 2023

And the non-breaking part could be enforced in CSS (white-space: nowrap).

Nice! Let's also make sure the label doesn't get wrapped by setting it as nowrap in CSS, since narrow puts a regular space and not a non-breaking one.

@kathirgounder
Copy link
Contributor Author

And the non-breaking part could be enforced in CSS (white-space: nowrap).

Nice! Let's also make sure the label doesn't get wrapped by setting it as nowrap in CSS, since narrow puts a regular space and not a non-breaking one.

Hi @mourner ! Thanks again for the review, I should've mentioned it in my update, seems like you guys already added the 'nowrap' property in the CSS file in this pr to tackle the same wrapping issue: https://github.com/mapbox/mapbox-gl-js/pull/11850/files#diff-ddbd4aa8b3bad8f105e53a268b99db49be5ad58f3db5e57719baf9afb9ddc9c3

white-space: nowrap;

If I need to enforce it somewhere else as well, please let me know! Thank you!

@mourner mourner marked this pull request as ready for review April 12, 2023 08:16
@mourner mourner requested a review from a team as a code owner April 12, 2023 08:16
@mourner mourner merged commit c76789e into mapbox:main Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants