svgdotjs / svg.js

The lightweight library for manipulating and animating SVG
https://svgjs.dev
Other
11.14k stars 1.08k forks source link

Text does not have width() #1077

Closed voxspox closed 4 years ago

voxspox commented 4 years ago

Bug report

If created a group g of different elements like text, circle, etc. I want to arrange this elements and iterate over g.children() and access width(). Unfortunately width is not available for all childs p.ex. text

const g = draw.group()
g.circle(10)
g.plain('Label')
...

let x = 0
for (let item of g.children()) {
  item.translate(x, 0)
  x += item.width() + 10
  // error if item is a <text>
}

I have the following workaround:

x += item.node.getBBox().width + 10
Fuzzyma commented 4 years ago

The width() method accesses the width attribute of the element. Text does not have this attribute. However, I see the incosistency because circle does have the method even though there is no width attribute. So I guess it is just missing.

Your workaround on the other hand is on point. I would generally advice to use bbox here because it works for all shapes the same way without any quirks done by the library. Just the way you get the box is unusual for svg.js because you can just go item.bbox().width

voxspox commented 4 years ago

ah ok. didn't recognize bbox() so far :)

voxspox commented 4 years ago

maybe you want to add a calcWidth() to Element.

Fuzzyma commented 4 years ago

Why would I when there is bbox? The fact that text misses the width attr might be a bug but there is no use for an extra method which just returns something from bbox

voxspox commented 4 years ago

bbox just wraps SVGs bbox and does no extra calculations, right? If bbox().width is always possible, i.e. bbox never fails, then I agree with you bbox is enough and there is no need for an extra function.

Fuzzyma commented 4 years ago

Correct, beside that, a width() implementation for text would do exactly that - because text itself does not have a width attribute. Another possibility to get the width is calling text.node.getComputedTextLength()

Fuzzyma commented 4 years ago

So I will close this issue because its not likely to get fixed. Thats because it is not possible to set a width/height or size like this. It would only work as getter. Since we want a unified api for all elements it is better to be explicit about not having width etc because it wont work for setting