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

Features: repeat distance, allowCrop, multi-color labels, orientation 'auto' and 'flip', turnedText, ... #72

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

plepe
Copy link

@plepe plepe commented Dec 24, 2018

I have a christmas present for you!

I would have liked to create pull requests for each feature, but they all depend on each other, that's why I created a "features" pull request.

It includes the following features:

  • repeat distance: You can specify repeat as float to set the distance between each repetition in pixels (will be approximated by spaces). Replaces repeat: specify distance in pixels #70 .
  • allowCrop: If the line is too short to display the whole text, crop the text. If false, don't show the text at all. (Default: true). This could improve performance on maps with short lines.
  • multi-color labels: (or better "labels with multiple tspans") A label can consist of several tspans, which can change all available attributes. See documentation of the extended text parameter. Fixes Flip upside down text on right to left lines...? #26 . (See the "Rainbow" in the screenshot below)
  • Orientation 'auto': Flip labels according to the orientation of the line (also for part of lines). Therefore labels can always be upright. (See the "Road" in the screenshot below)
  • Orientation 'flip': Fix Bug with orientation=flip #71 .
  • Option turnedText: When flipping a label because of orientation auto (for east-west line parts), use this text instead. Useful if a label includes directional characters like arrows.

Also:

Tell me what you think about it.

  • Are the options named properly?
  • Is the documentation understandable?
  • Can you detect any regressions?

rainbow-and-directional-labels

@plepe
Copy link
Author

plepe commented Jan 8, 2019

One big problem is browser support for rotated text on textpath.

  • Chrome-based browsers (Opera, ...): very good
  • Firefox: problems with fonts with variable width, also shifted by one character
  • IE / Edge: does not rotate labels

You can check how your browser behaves (first image) and compare with screenshots of other browser on this test page:
https://xover.mud.at/~skunk/svg-rotate-glyph

Please submit screenshots of other browsers!

I guess I should implement a browser detection and mitigate problems (e.g. select a fixed-width font on firefox, disable rotation on IE/Edge).

@clementallen
Copy link

Love this! Really hope it can get merged in soon 👍
If it helps, here is screenshot of the above website in Safari 12 on macOS Mojave:
screenshot 2019-01-13 20 49 42

@plepe
Copy link
Author

plepe commented Jan 14, 2019

Oh no, Safari does not seem to support textpath at all!

Do you see airplanes, labels and other markers on this webpage? https://makinacorpus.github.io/Leaflet.TextPath/

Apparently there seems to be a bug in Safari:
https://stackoverflow.com/questions/50937985/text-within-svg-textpath-not-visible-from-ios-browsers

@clementallen
Copy link

I can see the aircraft and other markers fine in safari:
screenshot 2019-01-14 19 18 05

@plepe
Copy link
Author

plepe commented Jan 14, 2019

That looks okay. I'll try to get my hands on a Mac soon, so I can test what's wrong with my test page ...

@clementallen
Copy link

clementallen commented Jan 14, 2019

Just cloned your fork and ran it on Safari. Everything seems to be fine apart from the turnText and orientation. Unfortunately I don't really know anything about SVG elements otherwise I'd try and fix it:

screenshot 2019-01-14 20 07 23

screenshot 2019-01-14 20 07 29

@simon1tan
Copy link

simon1tan commented Jan 30, 2019

@plepe Thanks for the new options! I tried orientation:'auto' but getting multiple errors:
Error: attribute transform: Expected number, "rotate(auto 0 0)".
Error: attribute transform: Expected number, "rotate(auto 489.8046875…"

@plepe
Copy link
Author

plepe commented Jan 30, 2019

@plepe Thanks for the new options! I tried orientation:'auto' but getting multiple errors:
Error: attribute transform: Expected number, "rotate(auto 0 0)".
Error: attribute transform: Expected number, "rotate(auto 489.8046875…"

Oh. Hard to tell what is the problem without knowing further details. Could you create a test case?

@simon1tan
Copy link

Sorry @plepe. That was my error. Didn't refresh. Please ignore that error.

```javascript
layer.setText([
{ fill: 'red', text: 'Red' },
' '

Choose a reason for hiding this comment

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

I think there's a comma missing here

var cacheId = JSON.stringify(text) + '|' + JSON.stringify(options.attributes)

if (cacheId in _getLengthCache) {
return _getLengthCache[cacheId]

Choose a reason for hiding this comment

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

Missing semicolon

var length = pattern.getComputedTextLength();
svg.removeChild(pattern);

_getLengthCache[cacheId] = length

Choose a reason for hiding this comment

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

Missing semicolon

@strixy
Copy link

strixy commented Feb 25, 2019

Good job! Thank you. I ran into one issue with the text not centering as expected, but otherwise super happy e-w text is displaying nicely now. As a fix I can propose the following solution, but I'm new here so I'll leave the final solution up to you.

Change line 109

from
if (!options.allowCrop) {
to
if (!options.allowCrop || options.center) {

var finalText = [];
var dx = 0

if (!options.allowCrop) {
Copy link

Choose a reason for hiding this comment

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

Fixes text center issue
if (!options.allowCrop || options.center) {

@plepe
Copy link
Author

plepe commented Feb 26, 2019

Thanks :-) The biggest problem right now is the browser incompatibilities. It will be necessary to implement some kind of switch, which mode to choose:

  • Full support: (Chromium)
  • Support with monospace font (Safari, Opera)
  • Support with monospace font, shifted by one character (Firefox)
  • No support (IE, Edge, possibly older browsers)

See: https://xover.mud.at/~skunk/svg-rotate-glyph/ (the relevant line is the "textpath with rotate=180" line)

(Safari also needs the deprecated 'xlink:' prefix when referencing the path)

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.

5 participants