-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat(Vis Libraries): Use new libraries #321
Conversation
package.json
Outdated
"vis": "4.21.0", | ||
"@types/vis": "4.21.19" | ||
"vis-network": "5.4.1", | ||
"vis-timeline": "5.1.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types for Timeline has been merged but not released yet. Maybe we should wait for the next version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i would like to wait for that
@@ -385,7 +383,7 @@ export class VisTimelineService { | |||
* | |||
* @memberOf VisTimelineService | |||
*/ | |||
public getWindow(visTimeline: string): { start: Date, end: Date } { | |||
public getWindow(visTimeline: string): { start: Date; end: Date } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the other projects we reformatted through separate PRs authored by vis-bot. It doesn't give attribution to people for running prettier --write
, follows one PR one thing filosophy and would also make reviewing easier by filtering out code style changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also would seperate style changes from other thinks. But I'm not sure if that's the case here. Also this is such a minor change that I have no problem with it.
components/network/index.ts
Outdated
export interface VisNodeOptions extends NodeOptions {} | ||
export interface VisPosition extends Position {} | ||
|
||
export class VisNodes extends DataSet<VisNode> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new data set types require id type to provide proper type safety: DataSet<VisNode, 'id'>
. Also you can simply use DataSetNodes
here instead of dealing with generic types.
components/network/index.ts
Outdated
export interface VisNetworkData extends Data {} | ||
export interface VisNode extends Node { | ||
title?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node already has title?: string
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed that
components/network/index.ts
Outdated
@@ -102,7 +118,7 @@ export class VisNodes extends Vis.DataSet<VisNode> { | |||
} | |||
} | |||
|
|||
export class VisEdges extends Vis.DataSet<VisEdge> { | |||
export class VisEdges extends DataSet<VisEdge> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As with nodes either DataSet<VisEdge, 'id'>
or DataSetEdges
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed that
components/network/index.ts
Outdated
@@ -102,7 +118,7 @@ export class VisNodes extends Vis.DataSet<VisNode> { | |||
} | |||
} | |||
|
|||
export class VisEdges extends Vis.DataSet<VisEdge> { | |||
export class VisEdges extends DataSet<VisEdge> { | |||
public constructor(data?: VisEdge[], options?: VisDataSetOptions) { | |||
super(data, options); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's exactly the point of this and the other methods that just take the arguments, pass them to the inherited method and then return it's return value? Would anything change if they weren't there?
I think @Thomaash should review this. I'm just starting with all that typescript and angular stuff... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @hypery2k, Vis Timeline has been released with types included. When you'll have time you can finish this.
Already added it locally, hopefully will find some time next week for this |
* Drop own types * adopt types/APIs see #321
I published a preview of the package for testing: |
see #314