search

svgdotjs / svg.js

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

Fix throwing when call svg.ungroup() #1234

Closed OpenGG closed 10 months ago

OpenGG commented 2 years ago

Current status: svg.ungroup() throws with confusing message "Uncaught TypeError: parent.screenCTM is not a function". Fix: Early throws with determine message.

Fuzzyma commented 2 years ago

Another possibility would be to just call flatten instead. But that might be confusing. We really rarely throw errors in the lib because validating all user input would quickly bloat the code. How did you discover this?

OpenGG commented 2 years ago

@Fuzzyma Am really just newbee digging around. If ungroup() not supposed to be called on svg root, maybe it should be moved to Group class?

Fuzzyma commented 2 years ago

Well you can call it on a nested svg. Also there are other container elements that you can ungroup. Basically everything that can have children (defs for example)

OpenGG commented 2 years ago

@Fuzzyma Then what should be done with this issue in your opinion? Option 1: .ungroup() can be call with "basically everything" In this case, early throwing or polymorphism? Both requires special handling with different elements. Option 2: Limit .ungroup() to g Relatively simple, may break flatten(). Option 3: Do nothing Calling .ungroup() with svg cause throwing, other elements may too.

Fuzzyma commented 2 years ago

Ungroup can ONLY be called for container elements. The only option is leave it or implement your fix. However your fix will break for other elements than svg because they don't have the isRoot method. So I guess we leave it for now