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

support CSS line-height for Label font #8954

Merged
merged 5 commits into from
Aug 17, 2020

Conversation

nmschulte
Copy link
Contributor

@nmschulte nmschulte commented Jun 12, 2020

  • adds support for controlling line-spacing in multi-line Labels -- avoids needing separate labels and new-line/transparent-background/fill hacks for certain multi-line labeling visualizations
  • changes multi-line Label line-spacing basis from per-label max-glyph-height to font-size -- e.g. when rendering a label with text "^1" at the same location as one with text "^", the carets will render at the same height (with certain alignments... i.e. left-bottom)
  • tested to support "0" line-spacing, though I think the line-spacing codes can still be improved to be consistent with CSS behaviors/expectations -- see maxLineHeight (understood as "line's max glyph height", excl.s spacing) and how it interacts with otherLinesHeight ("height of other lines", incl.s spacing) and totalLineHeight ("height of all lines", incl.s spacing); I think some of this codes should simply always use the specified font-size -- this implicates how the background is drawn, and the present logic specializes on the first or last line to avoid an "extra" line-spacing

@cesium-concierge
Copy link

Thanks for the pull request @nmschulte!

  • ✔️ Signed CLA found.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@@ -407,7 +407,7 @@ function repositionAllGlyphs(label) {
horizontalOrigin,
backgroundPadding
);
var lineSpacing = defaultLineSpacingPercent * maxLineHeight;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I question why line-spacing is based upon the maximum glyph height instead of the font-size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure either. Your calculation looks fine, but we just need to make sure letters than go below the base like like 'y' and 'g' don't overrun tall letters like 'T' on the line below it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The existing logic causes multi-line label "heights" to only be as tall as the tallest character in the string, instead of a fixed height based upon the font-size (which is how I understand CSS font-size to work).

So label with text _\n_\n_\n_ renders with effectively small "line-heights" compared to one with text M\nM\nM\nM.
image

@nmschulte nmschulte force-pushed the label-css-line-height branch 2 times, most recently from eb79777 to 8cdca14 Compare June 12, 2020 20:58
Source/Scene/Label.js Outdated Show resolved Hide resolved
changes multi-line Label line-spacing basis from per-label max-glyph-height to font-size
@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @nmschulte!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@nmschulte
Copy link
Contributor Author

@hpinkos is there anything I can do to get this included? Does the breaking/behavior change need more clarification, or just some close attention for consideration?

@hpinkos
Copy link
Contributor

hpinkos commented Aug 17, 2020

@nmschulte sorry, I just haven't had the bandwidth to get back to this. @kring or @lilleyse would you have time to review?

@kring
Copy link
Member

kring commented Aug 17, 2020

This seems like a good change to me. I tried a bunch of things and didn't run into any problems. Thanks @nmschulte!

@kring kring merged commit 13b40ad into CesiumGS:master Aug 17, 2020
@nmschulte nmschulte deleted the label-css-line-height branch August 18, 2020 17:10
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