Conversation
|
The only anoyance is the fact that transform = new ZoomTransform(p.k, p.x, p.y);
draw.setTransform(transform);
redraw();Maybe @weeman1337 has an idea here? |
weeman1337
left a comment
There was a problem hiding this comment.
🥳 thanks for migrating the project to TypeScript. Left some notes.
Also ESLint explodes, because it cannot parse TypeScript:
npm i --save-dev @typescript-eslint/parser- Add
"parser": "@typescript-eslint/parser"to.eslintrc.json
lib/container.ts
Outdated
| tag = "div"; | ||
| } | ||
|
|
||
| let self = { |
There was a problem hiding this comment.
If a variable is not reassigned, it should be declared as const
(counts for other places as well)
There was a problem hiding this comment.
I wanted to change as little as needed here to not make the changes more confusing than they already are but can change it at least for the self's
lib/forcegraph.ts
Outdated
|
|
||
| function transformPosition(p) { | ||
| function transformPosition(p: { k: number; x: number; y: number }) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
To solve this ignore transform should be replaced with a new instance
transform = new d3Zoom.ZoomTransform(p.k, p.x, p.y);
There was a problem hiding this comment.
I would love doing that but that breaks it even more, see the comments above.
Can you eventually show me exactly how you mean that at some point?
| let router = window.router; | ||
|
|
||
| if (node !== undefined) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Is node actually something different here?
There was a problem hiding this comment.
Is should be some kind of MapNode that has an o attribute with the actual node
|
|
||
| el.scrollIntoView(false); | ||
| el.classList.add("infobox"); | ||
| // @ts-ignore |
There was a problem hiding this comment.
If destroy() should be kept with the element, you could use an extended element with the destroy() function defined.
| h2.classList.add("proportion-header"); | ||
| h2.textContent = _.t(heading); | ||
| h2.onclick = function onclick() { | ||
| // @ts-ignore |
There was a problem hiding this comment.
renderSingle() should return an extended table with the elm prop.
weeman1337
left a comment
There was a problem hiding this comment.
Tested, looks good and works!
Description
This replaces most .js files with their typescript equivalent.
Motivation and Context
Having no clue what something does is bad, this fixes it by having types and thus at least some clues 🎉
How Has This Been Tested?
On my laptop, technically should produce the same JS as before
Screenshots/links:
n/a
Checklist: