svgdotjs / svg.js

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

Moving a group containing a title doesn't work #1102

Closed The4thLaw closed 1 year ago

The4thLaw commented 4 years ago

Bug report

Fiddle

https://jsfiddle.net/1dj9bzow/1/

Explanation

I'm adding a title to a group element as described in https://svgjs.com/docs/3.0/shape-elements/#element-words . According to the SVG specs, a title can be added to any container element, including groups. When I try to move the group as a whole, I get the following error in the console:

TypeError: t.bbox is not a function

I assume the library (v3) is trying to get the bbox of the title element.

Note that it seems to work properly on v2.7: https://jsfiddle.net/mfy8e9nd/

Thanks :)

Fuzzyma commented 4 years ago

The way groups are moved has been changed in 3.0 to make it more consistent with the other elements. That means, that svg.js will move the bbox of the element. For groups that means, that the elements in the group are moved since groups itself dont have coordinates. What you want is probably just translating the group (thats how group movement was handled in 2.7):

group.translate(100, 100)
The4thLaw commented 4 years ago

I'll try to change that but it feels more like a hack given that in, the docs, the section "Positioning" only mentions the move() method (and related coordinates-based methods). I'm positioning numerous groups with titles in my chart (it will probably represent 80% of the data), and I'm not sure translating everything is really the best way, semantics-wise.

Maybe the existing method for moving groups should accomodate elements that have no bbox. title is a standard element and it seems odd that svgjs would fail to handle it.

Fuzzyma commented 4 years ago

Well - you said yourself that it worked with 2.7 and didnt even realize that it was using a translation internally. What counts is the result in the end. It is only natural to use transformations. When you rotate something, you always transform, too. Beside that is translating the more performant way instead of figuring out the new position for every element in the group (especially if these are nested groups).

And yes, we need to add a switch to dodge elements which are descriptive elements.

The4thLaw commented 4 years ago

Actually I only noticed that it worked in 2.7 because the jsfiddle mentionned in the GitHub bug report template still uses v2.7. I was trying to make my SSCCE and couldn't reproduce the issue. I never checked what was produced ;)

Thanks for the reasoning behind the translation.

Fuzzyma commented 4 years ago

Actually I only noticed that it worked in 2.7 because the jsfiddle mentionned in the GitHub bug report template still uses v2.7

Woops - I really should update that

keyone2693z commented 3 years ago

@The4thLaw

is there any fix for this I'm facing the same issue

Fuzzyma commented 3 years ago

@keyone2693z the fix is to use translation for now. Its not really convenient. I am sorry!

keyone2693z commented 3 years ago

@Fuzzyma thanks the problem I have is that when I'm using element.words from the beginning of loading the SVG its give me the error TypeError: child.bbox is not a function

const gw = cg.group(); gw.transform({ translateX: BaseRenderer.baseWidth + 15, translateY: (BaseRenderer.baseHeight - 20) / 2 }); gw.image(......svg).size(20, 20); gw.element('title').words('is invalid');

where should I put the translation

The4thLaw commented 3 years ago

@keyone2693z I think you need to use gw.translate(BaseRenderer.baseWidth + 15, (BaseRenderer.baseHeight - 20) / 2). Not transform with translate parameters.

Fuzzyma commented 3 years ago

@The4thLaw its just another syntax of the same translation. But its really weird that you get the error there...

keyone2693z commented 3 years ago

ya, we just downgraded to the 2.7