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

Missing methods on SVG.FX in svg.js.d.ts #597

Closed
peterennis opened this issue Feb 21, 2017 · 32 comments
Closed

Missing methods on SVG.FX in svg.js.d.ts #597

peterennis opened this issue Feb 21, 2017 · 32 comments

Comments

@peterennis
Copy link

Installed using Ionic2.
First test basic animation worked.
Rotate fail.
v2.4.0

Apart from the error my goal is to load an SVG icon and animate it
but did not see how to do that in the docs.

capture009
capture010
capture011

@dotnetCarpenter
Copy link
Member

@peterennis Aside from .rotate() missing from .animation().

You can animate the paths of a logo by morphing between a start path and an end path. Here is an example with two similar icons from icomoon: http://codepen.io/dotnetCarpenter/pen/mREvjL

If you need to animate/morph between less similar paths, you can use svg.pathmorphing.js.

@dotnetCarpenter
Copy link
Member

After reading the docs, I think you should use .attr({ rotate: 45 }). http://svgjs.com/animating/animate/

I will check later if this is a bug or a documentation bug.

@Fuzzyma
Copy link
Member

Fuzzyma commented Feb 23, 2017

@dotnetCarpenter your code will set an attribute rotation on an element which just willl do nothing.
Just rotate like you would normally do.

svgIcon.animate().rotate(45)
//or
svgIcon.transform({ rotate: 45 })

To have relative transformations (which is maybe what you are looking for), use the relative flag:

svgIcon.transform({ rotate: 45 }).transform({ skewX:25, skewY:0 }, true)

EDIT: for the bug which says that rotation is not defined on animation: Fell free to create a PR and fix that in the ts file

@dotnetCarpenter
Copy link
Member

@peterennis I just noticed that you're using svgjs instead of SVG. How do you import SVG.js?

@dotnetCarpenter
Copy link
Member

@peterennis It appears that this is not a bug. I've just made a demo with your use-case and it works fine: https://jsfiddle.net/dotnetCarpenter/5yjc8k84/

@Fuzzyma thanks for the corrections.

@dotnetCarpenter
Copy link
Member

This might be an issue with svg.js.d.ts.

@dotnetCarpenter
Copy link
Member

dotnetCarpenter commented Feb 24, 2017

@peterennis Can you patch svg.js.d.ts with rotate(degrees: number, cx: number, cy: number): Animation on line 952 and see if that solves your problem?

If yes, then I know how to fix typings. But since I don't use typings, I need you to confirm that it is working.

@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 24, 2017

I think there might be a lot more methods than rotate that cause problem as this comment on svg.js.d.ts point out. We will definitely need to review this file.

@peterennis
Copy link
Author

@dotnetCarpenter I will look into this over the weekend and get back to you.
The fiddle is what I am expecting to see on my end.
I notice you are working towards a 2.5 release so going forward it looks like keeping
sync with typings becomes a new task.

@dotnetCarpenter
Copy link
Member

@peterennis yes you're right. Perhaps we can all hack something together. There is an interesting answer on SO for that, here: http://stackoverflow.com/questions/12687779/how-do-you-produce-a-d-ts-typings-definition-file-from-an-existing-javascript#12695001

@dotnetCarpenter dotnetCarpenter changed the title Property rotate does not exist on type Animation Missing methods on SVG.FX in svg.js.d.ts Feb 25, 2017
@peterennis
Copy link
Author

@dotnetCarpenter said:
"Can you patch svg.js.d.ts with rotate(degrees: number, cx: number, cy: number): Animation on line 952 and see if that solves your problem?"

No, it did not fix the problem.
image
image

@peterennis
Copy link
Author

@dotnetCarpenter
Steps to include svg.js in Ionic2

I am using Windows 10 with Visual Studio Code

  1. npm install svg.js --save
  2. npm install --save @types/svg.js
  3. In the ts page: import * as svgjs from 'svg.js';
  4. The ionViewWillEnter function in the ts page will kick off the animation

I do not know enough to help with typings, but I can test...

@peterennis
Copy link
Author

@Fuzzyma
Same problem with transform.

image
image

@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 25, 2017

@peterennis: Could you try rotate(degrees: number, cx?: number, cy?: number): Animation ? I think this should work.

I'm not surprised that transform does not work as it is also missing in svg.js.d.ts.

@dotnetCarpenter
Copy link
Member

@RmiTtro, @peterennis if rotate(degrees: number, cx?: number, cy?: number): Animation works, then we can easily patch up the rest of the missing methods.

@peterennis
Copy link
Author

@RmiTtro, @dotnetCarpenter
This took it to the next step.
I still get the red squiggle under rotate but the function worked.
Skew was next in line to fail.

capture018
capture019
capture020
capture021

@peterennis
Copy link
Author

image

@peterennis peterennis mentioned this issue Feb 25, 2017
10 tasks
@RmiTtro
Copy link
Collaborator

RmiTtro commented Feb 26, 2017

Thanks for testing this out! We are making progress, that's good! For skew, I think that something like this should work: skew(x: number, y?: number, cx?: number, cy?: number): Animation. Although I'm afraid that if you plan to make a lot of animations with SVG.js you are going to find a lot more methods that do not work as there a lot that are missing from svg.js.d.ts.

I'm a bit puzzled as to why there is still a red squiggle under rotate...

@peterennis
Copy link
Author

I have been doing some research into typings.

Quick summary:
Began with Definitely Typed (https://github.com/DefinitelyTyped)
Moved to Typings (https://github.com/typings/typings)
Now resident on NPM with the goal of "seamless integration" in packages
SO explanation: http://stackoverflow.com/questions/37548066/typescript-typings-in-npm-types-org-packages
Future: https://blogs.msdn.microsoft.com/typescript/2016/06/15/the-future-of-declaration-files/

Current definitions I am using are 0.0.17 (https://www.npmjs.com/package/@types/svg.js)

Fuzzyma ticket: typings/registry#574
svg.js Definitely Typed issue: DefinitelyTyped/DefinitelyTyped#9454

Automatic Typings Acquisition: https://github.com/Microsoft/nodejstools/wiki/Automatic-Typings-Acquisition
This is also happening in the latest Visual Studio Code

Go here: https://microsoft.github.io/TypeSearch/
and type svg.js in the search

Microsoft have a developer on this working directly with the Definitely Typed guys:
https://social.msdn.microsoft.com/profile/Ryan++Cavanaugh

TypeScript 2.2 is now available: http://www.typescriptlang.org/

The package owner should be in control of this process.
Follow up here:
https://github.com/Microsoft/dts-gen
with Ryan Cavanaugh

It should save you a bunch of heartache and get some automation
into the process.

Ryan should be able to answer the squiggle question...

@RmiTro, I will play with skew in my sandbox. Thanks.

HTH

@dotnetCarpenter
Copy link
Member

@peterennis thanks for the info. I'm afraid this will have to wait to after the 2.5.0 release but your last post is great starting point. If you are able to help further we'll appreciate it 👯‍♂️

@peterennis
Copy link
Author

I tested dts-gen, ran into problems and opened a ticket with Ryan Cavanaugh.
I could escalate it further to MS support and the Typescript team with backing from the svg.js
project lead. Add a confirming note to this ticket and I will get the ball rolling.

@dotnetCarpenter
Copy link
Member

dotnetCarpenter commented Mar 20, 2017

@peterennis Sorry for the delay. You have my full support. Let us know, if you need anything in particular from us.

ping: @wout, @Fuzzyma

@peterennis
Copy link
Author

peterennis commented Mar 31, 2017

@dotnetCarpenter, I looked into this some more and the layout of svg.js with code modules under src is not recognized by dts-gen. So I tried just doing that file by itself, but npm want fx.js installed as a module and there is actually an unrelated npm module called fx.

My next idea would be do take the fx.js file and see if that could be turned into an npm module (fxsvg ?),
install that and then run the generator. Unfortunately my npm foo is not that good so someone else would need to pick up the ball from here or else manual generation seems to be the final resort.

capture052

@dotnetCarpenter
Copy link
Member

We will be moving this thread to the proper department and expect a reply in one (1) business day.

Seems that ain't gonna happen (more than a week ago).

Gonna look into dts-gen.

@dotnetCarpenter
Copy link
Member

As I see it, dts-gen can not understand our architecture. When required via CommonJS, dts-gen -m svg.js -s, only export = svg.js; is outputted.

Doing it on each source file, won't work because our source files are meant to be concatenated as defined in our gulpfile.js. I've been championing es6 modules for a while, which might give dts-gen something to work with. E.i. if you try to use dts-gen on a single file, dts-gen -i "$(cat src/arrange.js)" -s, you get ReferenceError: SVG is not defined, which is correct.

Alternatively trying to feed all of SVG.js, dts-gen -i "$(cat dist/svg.js)" -s, will give you Argument list too long. While feeding the minified version just gives you nothing.

Even with a new architecture for SVG.js, which dts-gen understand, it will not know which arguments are optional of know which data type we expect.

I'm afraid that the typings definition has to be manually edited and kept ajoure. And I'm afraid we don't have this expertise in the team at the moment. I for one, don't even know how to enable typings for SVG.js in vscode.

@JackLeEmmerdeur
Copy link
Contributor

Although there has been happening nothing here for quite some time, I'm interesting in hearing if my problem is connected to the problems you have with dts-gen.

So far the Typescript support works splendorous and makes autocomplete a breeze in vscode.
It seems that there are still some TODOs left in svg.js.d.ts. I e.g. added transform(t: Transform, relative?: boolean): Element; to interface Animation in line ~1014.

Would it help to fork svg.js and whenever I would miss something in svg.js.d.ts, to add the typedefinition and create a pull request in the upstream repository?

@Fuzzyma
Copy link
Member

Fuzzyma commented Jul 5, 2018

Jeah I guess there havent been much progress lately. However behind the curtain we are hard working on 3.0 which comes with new amazing stuff and es6. I dont know if dts-get can keepup with our mixings (they would need to create the objects and look at their prototype - i dont know if they do that) but its cool anyway :D.

For your collaboration: Yes, create as much PRs as much as you want. That would help tremendously!

@JackLeEmmerdeur
Copy link
Contributor

Thanks for the confirmation regarding to forking. I'll do it so.

My intention wasn't to say that you haven't made progress. That is obviously disproved when browsing the git-history of the project. I just meant this specific issue. Sorry for the phrasing.

As for me, you're doing great work and I'll continue to use the library and looking forward to see v3.0

@Fuzzyma
Copy link
Member

Fuzzyma commented Jul 5, 2018

Hehe that was by no means what I wanted to imply :D. I just wanted to say that we move to es6 soon and then we might be able to use dts-gen and solve this issue with typescript.

Until then your contribution to the typings is very welcome! :)

@JackLeEmmerdeur
Copy link
Contributor

I discovered only two missing Typescript-definitions so far, as I only have used as small subset of functionality of svg.js.

Would it be better to create small pull-requests whenever I added something, or rather wait till I think I added a considerable number of missing definitions? If the former is a common practice, please excuse my question, as I don't have a lot of experience with collaboration on git.

Best wishes for the es6 migration.

@Fuzzyma
Copy link
Member

Fuzzyma commented Jul 7, 2018

Do as you like. You can go ahead and create a PR and push to that branch whenever you find something new. And I can merge whenever I feel like :D. But there is nothing wrong with small PRs in general.

@Fuzzyma Fuzzyma added the wontfix label Nov 1, 2019
@Fuzzyma Fuzzyma closed this as completed Nov 1, 2019
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

5 participants