-
Notifications
You must be signed in to change notification settings - Fork 375
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
TSify most of ReactViews/Custom/Chart #7421
base: main
Are you sure you want to change the base?
TSify most of ReactViews/Custom/Chart #7421
Conversation
@@ -102,7 +102,9 @@ class ChartPanel extends React.Component { | |||
<div className={Styles.holder}> | |||
<div className={Styles.inner}> | |||
<div className={Styles.chartPanel} style={{ height: height }}> | |||
<div className={Styles.body}> | |||
<div // @ts-expect-error No Styles.body defined. |
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.
Not sure how to solve these Styles.X being undefined, so I've added ts-expect-error
for now.
interface LineChartProps { | ||
id: string; | ||
chartItem: ItemType; | ||
scales: { x: (p: number) => number; y: (p: number) => number }; |
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 propTypes type was "object", is this the right TypeScript type for scales
?
return this.xScale(this.pointsNearMouse[0].point.x); | ||
return this.mouseCoords && this.mouseCoords.x; | ||
} | ||
|
||
@computed | ||
get pointsNearMouse() { | ||
if (!this.mouseCoords) return []; | ||
const coords = this.mouseCoords; | ||
if (!isDefined(coords)) return []; |
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 assumed this.mouseCoords
is not updated during the time this function runs, so the undefined
part of the type could be eliminated before doing the map over the chartItems.
// See: https://stackoverflow.com/questions/21753126/d3-js-starting-and-ending-tick | ||
scale={scale.nice()} | ||
// @ts-expect-error Wrong type for children in AxisProps vs React. |
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 wasn't sure what to do about this error and I couldn't find a way to put the ts-expect-error
in a place that mattered, so I pulled out the children and passed that as a separate property that I could ts-expect-error
. This triggered a ESLint warning, which I have also ignored. I have no idea about the consequences of this.
@@ -322,28 +330,31 @@ class Chart extends React.Component { | |||
</Group> | |||
</svg> | |||
<Tooltip {...this.tooltip} /> | |||
<PointsOnMap terria={terria} chartItems={this.chartItems} /> | |||
{this.chartItems.map((chartItem: ChartItem) => |
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.
Had to change this one completely compared to what was there before.
I followed the pattern of mapping over things from line 292, and then made pointOnMap
return null when there wasn't a point on map. I think the old code is broken, but I have no idea if what I've done is any better.
function findNearestPoint(points, coords, xScale, maxDistancePx) { | ||
function distance(coords, point) { | ||
function findNearestPoint( | ||
points: any[], |
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.
Is there a type for points somewhere?
e8e5941
to
576a253
Compare
576a253
to
0e30aac
Compare
What this PR does
Depends on: #7419, #7420
Change propTypes to an interface, add a constructor that calls
super
andmakeObservable
, and add: any
as type annotation in various places to make the files go through tsc.This passes the tests for me when the changes from #7419 and #7420 are present.
Test me
Not sure how to test this.
Checklist
doc/
.