Conversation
|
apata
force-pushed
the
main-graph-to-api-v2--double-headed
branch
2 times, most recently
from
cddaaaa to
2d27729
Compare
apata
force-pushed
the
main-graph-to-api-v2--double-headed
branch
from
916c50b to
91df8c6
Compare
|
|
apata
changed the base branch from
main-graph-to-api-v2
to
main-graph-v2-backend
|
apata
force-pushed
the
main-graph-to-api-v2--double-headed
branch
3 times, most recently
from
b590145 to
c37fa8f
Compare
apata
force-pushed
the
main-graph-to-api-v2--double-headed
branch
2 times, most recently
from
47d0f68 to
df87958
Compare
apata
force-pushed
the
main-graph-to-api-v2--double-headed
branch
from
7257733 to
6a3120a
Compare
| const browserDateFormat = Intl.DateTimeFormat(navigator.language, { | ||
| hour: 'numeric' | ||
| }) | ||
|
|
||
| export function is12HourClock() { | ||
| return browserDateFormat.resolvedOptions().hour12 | ||
| } | ||
|
|
There was a problem hiding this comment.
Copied from date-formatter.js that was deleted.
| // enter delay on mobile is needed to prevent the tooltip from entering when the user starts to y-pan | ||
| // but the y-pan is not yet certain | ||
| enter={isTouchDevice ? 'transition-opacity duration-0 delay-150' : ''} |
There was a problem hiding this comment.
This is a nice improvement on mobile! Holding a finger a bit longer on the graph is quite intuitive and it's much better to not let any tooltips show up to just disturb the experience.
The only thing that's bothering me a bit is that text outside the graph container (precisely the Channels tab text) gets selected when I tap-and-hold inside the graph container, which in turn makes the OS render a magnifying glass for text selection right next to my finger (instead of the actual tooltip). When I move my finger a bit, it displays the tooltip though.
^ something to fix another day I guess. Nice work!
| metrics: MetricValues // one item | ||
| } | ||
|
|
||
| export type MetricValues = [null] | [number] | [RevenueMetricValue] |
There was a problem hiding this comment.
Could we also export a MetricValue type here? We might even want to use a single type across the whole dashboard in the future, but for now we could just export it from here:
| export type MetricValues = [null] | [number] | [RevenueMetricValue] | |
| export type MetricValue = null | number | RevenueMetricValue | |
| export type MetricValues = [MetricValue] |
Then the inlined RevenueMetricValue | number | null types in main-graph.tsx and main-graph-data.ts could be replaced with MetricValue as well.
| // can't be done in a single pass with remapAndFillData | ||
| // because we need the xLabels formatting parameters to be known | ||
| const remappedDataInGraphFormat = remappedData.map( |
There was a problem hiding this comment.
If I'm reading this right, the only reason for not being able to find yMax in a single pass with remapAndFillData is knowing how to evaluate shouldShowDate and shouldShowYear, which come from first/last time label from main/comparison results.
Since meta.time_labels are the source of truth of x-labels, couldn't we just do:
mainFirstLabel = meta.time_labels[0]
mainComparisonLabel = meta.comparison_time_labels[0]
(and same for the last labels of both)?
| statsQuery.date_range = DashboardPeriod.realtime_30m | ||
| } | ||
|
|
||
| statsQuery.include.present_index = true |
There was a problem hiding this comment.
This is unused now, right? Not pushing to get rid of it yet, but just wanted to flag it. Let's see if the new dotted line definition works out first...
| getChange | ||
| }: { | ||
| data: MainGraphResponse | ||
| getNumericValue: (metrics: RevenueMetricValue | number | null) => number |
There was a problem hiding this comment.
This is one of those cases (but there are more) where we could replace the inlined type with a proper MetricValue. Also, metricValue would be a better argument name in this function too:
| getNumericValue: (metrics: RevenueMetricValue | number | null) => number | |
| getNumericValue: (metricValue: MetricValue) => number |
| const isPartial = partialTimeLabels.find((l) => l === timeLabel) | ||
| ? true | ||
| : false |
There was a problem hiding this comment.
I think this would do the trick too?
| const isPartial = partialTimeLabels.find((l) => l === timeLabel) | |
| ? true | |
| : false | |
| const isPartial = partialTimeLabels.includes(timeLabel) |
|
|
||
| export type LineSegment = { | ||
| startIndexInclusive: number | ||
| stopIndexExclusive: number |
There was a problem hiding this comment.
Non-blocking suggestion (feel free to ignore): Maybe use shorter names here. E.g.: fromIndex and toIndex without specifying whether the index is inclusive or exclusive?
| } | ||
|
|
||
| /** | ||
| * Creates segments from points of main series. |
There was a problem hiding this comment.
| * Creates segments from points of main series. | |
| * Creates segments from points of a series. |
Used for comparisons too.
| * No line is drawn from or to gaps in the data. | ||
| */ | ||
| export function getLineSegments(data: SeriesValue[]): LineSegment[] { | ||
| return data.reduce((segments: LineSegment[], curr, i) => { |
There was a problem hiding this comment.
Using reduce here makes it really difficult (at least for me) to understand this function. Had a long pause reading it. How about a simple for i loop?
I think it can also be simplified due to not needing to account for gaps appearing in the middle of the series anymore. Meaning that we can just break out of the loop on the first encounter of a !isDefined datapoint.
| width: number | ||
| marginRight: number | ||
| minClientX: number | ||
| maxAttempts?: number |
There was a problem hiding this comment.
I think this doesn't need to be an argument? It should always be a fixed value of 2, right? First try to render the y axis labels, and if it didn't fit, do it a second time with a correct width that fits all labels.
| if (typeof onPointerMove !== 'function') { | ||
| return | ||
| } |
There was a problem hiding this comment.
This appears to be redundant because GraphProps doesn't allow it to not be a function.
| .attr('offset', '100%') | ||
| .attr('stop-color', stopBottom.color) | ||
| .attr('stop-opacity', stopBottom.opacity) | ||
| return id |
There was a problem hiding this comment.
The return value is unused. Could make this function return void.
| new Array(maxXTicks).fill(null).forEach((_v, i) => { | ||
| const tickValues = scale.ticks(maxXTicks - i) | ||
| if (tickValues.every(isWholeNumber)) { | ||
| result.add(JSON.stringify(tickValues)) | ||
| } | ||
| }) | ||
| return result |
There was a problem hiding this comment.
Let's replace with a simple for i loop?
for (let i = 0; i < maxXTicks; i++) {
const tickValues = scale.ticks(maxXTicks - i)
if (tickValues.every(isWholeNumber)) {
result.add(JSON.stringify(tickValues))
}
}3 participants
Refactors line graph to Typescript, from chart.js library to d3.js library. Depends on the new query backend. #6173
The rationale for switching the library:
we need d3.js for the complicated world map (can't be done with chart.js)
having two charting libraries increases the size of dashboard javascript and therefore makes the website take longer to load
Test side-by-side
Remove console.log of "effect running"
Delete line-graph.js (-301)
Delete graph-util.js (-127)
Delete graph-tooltip.js (-216)
Delete date-formatter.js (-115)
Changes
Fixes
Tests
Changelog
Documentation
Dark mode