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

Text.tspan("xy").newLine(): undocumented and unexpected behavior #1088

Closed
d4h0 opened this issue Feb 27, 2020 · 7 comments
Closed

Text.tspan("xy").newLine(): undocumented and unexpected behavior #1088

d4h0 opened this issue Feb 27, 2020 · 7 comments

Comments

@d4h0
Copy link

d4h0 commented Feb 27, 2020

Today I experimented with text in svg.js and it seems newLine() doesn't have enough documentation (and the behavior was unexpected for me).

Here is the example from the documentation for Text:

var text = draw.text(function(add) {
  add.tspan('Lorem ipsum dolor sit amet ').newLine()
  add.tspan('consectetur').fill('#f06')
  add.tspan('.')
  add.tspan('Cras sodales imperdiet auctor.').newLine().dx(20)
  add.tspan('Nunc ultrices lectus at erat').newLine()
  add.tspan('dictum pharetra elementum ante').newLine()
})

I'd expect the following regarding the first three spans:

Lorem ipsum dolor sit amet
consectetur.

But the result is:

Lorem ipsum dolor sit amet consectetur.

To me (and probably many other people) 'newLine' means "insert new line character" or "break line". But it seems 'newLine' here means "create new line with this tspan".

I think another name than 'newLine' would have been better here to reduce confusion (for example 'toNewLine' or 'asNewLine').

Even more strange is that a tspan without 'newLine' at the end doesn't show up in the visible area of the SVG. Why does this happen?

I think the documentation needs to be updated to explain what 'newLine' actually does, and how tspans after a 'newLine' behave.

Besides, that, wouldn't there be other APIs that are less confusing?

For example, if 'Text' had a 'newLine' function, we could do the following:

var text = draw.text(function(add) {
  add.newLine(
    add.tspan('Lorem ipsum dolor sit amet ')
    add.tspan('consectetur').fill('#f06')
    add.tspan('.')
  )

  // Or:

  add.newLine().dx(20).tspan('Cras sodales imperdiet auctor.')
  add
    .newLine()
    .tspan('Nunc ultrices lectus at erat')

  // Or

  add.newLine('dictum pharetra elementum ante')
})

I don't know if this would be easy to archive, but to me, this seems like a much more userfriendly and intuitive API.

What do you think?

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 29, 2020

Yes maybe the naming is a bit off. The json representation of this node marks a line as "newLined" which would be already a better naming. The way newlines are realized is best done by having this method on a tspan that starts a new line. This might be a bit odd but everything else might get out of hand. Beside that, your last syntax proposal looks like it is easily implementable. And since it does not breaks any old behavior we could even ship it as a feature.
In that case newLine() would be a fancy way of creating a new tspan with newLine already called on it. You could then somehow combine it with the first approach (which btw is full of syntax errors :D):

add.newLine(function(line) {
  line.tspan(...)
})

This would then lead to nested tspans which is allowed by the specs. However, calling newLine on those tspans wouldnt have any effect.

@d4h0
Copy link
Author

d4h0 commented Mar 1, 2020

Beside that, your last syntax proposal looks like it is easily implementable. And since it does not breaks any old behavior we could even ship it as a feature.

Sounds good.

So basically, the new 'newLine' would be just a wrapper around 'tspan' which calls 'newLine' and returns the tspan, correct?

Somehow I think 'newLine' as the name for this new method would be confusing (because now there would be two methods (on 'Text' and 'tspan') with the same name but different behauviour, if I understand everything correctly).

Maybe just 'line' would be okay?

In this context, I'd prefer this name anyway.

This would then lead to nested tspans which is allowed by the specs. However, calling newLine on those tspans wouldn't have any effect.

Would 'newLine' on inner tspans automatically have no effect, or would there need to be a check in 'newLine'?

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 2, 2020

So basically, the new 'newLine' would be just a wrapper around 'tspan' which calls 'newLine' and returns the tspan, correct?

correct!

Somehow I think 'newLine' as the name for this new method would be confusing (because now there would be two methods (on 'Text' and 'tspan') with the same name but different behauviour, if I understand everything correctly).

I agree!

Would 'newLine' on inner tspans automatically have no effect, or would there need to be a check in 'newLine'?

When building our lines with svg.js we only check the first level of tspans. So calling newLine on a deeper nested tspan would just not recognized by the function building the lines. It is silently ignored. Therefore nothing "bad" happens. A check is unneccesary.

@d4h0
Copy link
Author

d4h0 commented Mar 4, 2020

Okay, sound pretty simple.

Will you implement this?

Or should I give it a try? :)

@Fuzzyma
Copy link
Member

Fuzzyma commented Mar 5, 2020

Well you can if you want. But only if you see any benefit for you with that. I am in the process of creating a new release anyway atm. So it might be faster when I do it instead of reviewing a PR forth and back :D

@d4h0
Copy link
Author

d4h0 commented Mar 7, 2020

So it might be faster when I do it instead of reviewing a PR forth and back :D

I agree, and there wouldn't be a significant benefit for me personally (now that I know how 'newline()' works).

Thanks for your work :)

Fuzzyma added a commit that referenced this issue Mar 28, 2020
### Fixed
 - fixed `zoom()` method of runner which was passed a wrong parameter
 - fixed positioning methods of `TSpan` to position them by its bounding box
 - fixed `flip()` method which flips correctly by center by default now and accepts correct arguments
 - fixed a case in `rbox()` where not always all values of the box were updated
 - fixed `getOrigin()` function used by `transform()` so that all origin (#1085) popssibilities specified in the docs are working
 - fixed positioning of text by its baseline when using `amove()`
 - fixed tons of typings in the svg.d.ts file

### Added
 - added second Parameter to `SVG(el, isHTML)` which allows to explicitely create elements in the HTML namespace (#1058)
 - added `unlink()` and `linker()` to hyperlinked elements to remove or access the underling `<a>` element
 - added `wrap()` method to `Dom` which lets you wrap an element by another one
 - added `orient()` method to `Marker`
 - added `options` parameter to `dispatch()` and `fire()` to allow for more special needs
 - added `newLine()` constructor to `Text` to create a tspan marked as new line (#1088)
 - added lots of tests in es6 format
@markocrni
Copy link

I comment in this section, but this is probably a problem for itself, and again it refers to tspans, and their dx value. I've been working with svg that isn't attached to the DOM, and I've come to the conclusion that this part is causing the problem (tspan):
const fontSize = globals.window.getComputedStyle(this.node)
Since SVG is not part of DOM, getComputedStyle cannot read font size and value always will be 0. Setting value by .dx(20) also doesn't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants