-
Notifications
You must be signed in to change notification settings - Fork 121
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: add cursor sync mechanism #304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #304 +/- ##
=========================================
- Coverage 98.13% 98.04% -0.1%
=========================================
Files 37 37
Lines 2687 2714 +27
Branches 634 626 -8
=========================================
+ Hits 2637 2661 +24
- Misses 45 48 +3
Partials 5 5
Continue to review full report at Codecov.
|
Not sure if the issue elastic/kibana#14799 will be fixed by this, the sync still seems pretty slow. @markov00 any ideas? |
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 think we have to find a different way to implement that mechanism, the problem is the following:
the onCursorUpdateListener
push out cursor positions as fast as it can, but we are limited by the slowness of setting the activeCursor
value. This because we are continuously updating the specs on every cursor change triggering on every new setting change a computeChart
.
With the refactoring I'm working on that will not be a big problem, but right now it still a limitation.
There is also another problem with the implementation: in Kibana we have to wire up this logic of cursorUpdate callback and activeCursor between completely separated elements, that can be a bit problematic: how will keep the state of the activeCursor
?
I think that can be solved directly updating the state instead of going through the settings update. It can be achieved creating method on the Chart
component that update the setCursorValue
when called, and calling that from a another chart ref like the following:
// playground.tsx
export class Playground extends React.Component {
state = {
cursor: undefined,
}
ref1 = React.createRef<Chart>();
ref2 = React.createRef<Chart>();
ref3 = React.createRef<Chart>();
ref4 = React.createRef<Chart>();
componentDidMount() {
}
onCursorUpdate = (cursor: Cursor) => {
this.ref1.current.dispatchExternalCursorEvent(cursor);
this.ref2.current.dispatchExternalCursorEvent(cursor);
this.ref3.current.dispatchExternalCursorEvent(cursor);
this.ref4.current.dispatchExternalCursorEvent(cursor);
}
render() {
return (<React.Fragment>
{renderChart('1', this.ref1, KIBANA_METRICS.metrics.kibana_os_load[0].data.slice(0, 15), this.onCursorUpdate)
}
{renderChart('2', this.ref2, KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 15), this.onCursorUpdate)
}
{renderChart('3', this.ref3, KIBANA_METRICS.metrics.kibana_os_load[2].data.slice(15, 45), this.onCursorUpdate)
}
{renderChart('4', this.ref4, KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 100), this.onCursorUpdate)
}
</React.Fragment>)
}
}
function renderChart(key: string, ref: React.RefObject<Chart>, data: any, onCursorUpdate?: CursorUpdateListener) {
return (
<div key={key} className="chart">
<Chart ref={ref}>
<Settings debug={false} showLegend={true} onCursorUpdate={onCursorUpdate} />
<Axis
id={getAxisId('timestamp')}
title="timestamp"
position={Position.Bottom}
tickFormat={niceTimeFormatter([1555819200000, 1555905600000])}
/>
<Axis id={getAxisId('count')} title="count" position={Position.Left} tickFormat={(d) => d.toFixed(2)} />
<LineSeries
id={getSpecId('dataset A with long title')}
xScaleType={ScaleType.Time}
yScaleType={ScaleType.Linear}
data={data}
xAccessor={0}
lineSeriesStyle={{
line: {
stroke: 'red',
opacity: 1,
},
}}
yAccessors={[1]}
/>
<LineSeries
id={getSpecId('dataset B')}
xScaleType={ScaleType.Time}
yScaleType={ScaleType.Linear}
data={KIBANA_METRICS.metrics.kibana_os_load[1].data.slice(0, 15)}
xAccessor={0}
yAccessors={[1]}
stackAccessors={[0]}
/>
</Chart>
</div>);
}
// chart.tsx
...
export class Chart extends React.Component<ChartProps> {
...
dispatchExternalCursorEvent(event: Cursor) {
if (event && event.chartId !== this.chartSpecStore.activeChartId) {
this.chartSpecStore.setCursorValue(event.value);
}
}
render() {
const { renderer, size, className } = this.props;
let containerStyle: CSSProperties;
if (size) {
....
This is a just a possible implementation but works without going through the spec parsing and chart computing every time.
Refactor activeCursor logic to call method on Chart to update cursor and not rely on prop to Settings component.
@markov00 I updated the logic as we discussed. Much better performance 👍 I also added back the Demo |
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've added few comments that should be addressed before merge.
Do you think you can also add some more testing to have a green/positive diff coverage?
@@ -23,6 +26,17 @@ interface TooltipProps { | |||
headerFormatter?: TooltipValueFormatter; | |||
} | |||
|
|||
export interface CursorEvent { |
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.
You missed to export that type from the main index.ts
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.
Can you also please add a bit more documentation for that interface?
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.
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.
changes LGTM
# [9.1.0](v9.0.4...v9.1.0) (2019-08-14) ### Features * add cursor sync mechanism ([#304](#304)) ([c8c1d9d](c8c1d9d))
🎉 This PR is included in version 9.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [9.1.0](elastic/elastic-charts@v9.0.4...v9.1.0) (2019-08-14) ### Features * add cursor sync mechanism ([opensearch-project#304](elastic/elastic-charts#304)) ([3986dfc](elastic/elastic-charts@3986dfc))
Summary
Add
CursorUpdateListener
as props onSettings
component to allowChart
consumer to synchronize cursors across multipleChart
s by callingdispatchExternalCursorEvent
on theChart
ref to update cursor value.Issue #83
Demo
Checklist
Chart
s sharing state)