Skip to content
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

Ruler interaction replacing Grid mouse listeners for comparative tooltips #499

Merged
merged 46 commits into from
Apr 27, 2020
Merged
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
42c8eaf
add ruler and axis tooltip
lucafalasco Feb 11, 2020
2b778ba
move ruler logic to it's own graphFrameComponent
lucafalasco Feb 13, 2020
cf6eeb2
refactor, add axis tooltip dynamic width, add matching elements hovering
lucafalasco Feb 18, 2020
6c1bfa5
use getDomainValue instead of hardcoded accessor
lucafalasco Feb 19, 2020
269ab2a
add snap to nearest point
lucafalasco Feb 19, 2020
7480ff6
use getSVGElementSize to get backdrop height
lucafalasco Feb 20, 2020
3c56aa0
remove useless code
lucafalasco Feb 20, 2020
be5e962
add trigger mouseout in hideRuler to only highlight single hovered el…
lucafalasco Feb 20, 2020
0aefa01
WIP
lucafalasco Feb 24, 2020
2026a0d
fix categorical scale elements mouseover trigger
lucafalasco Feb 25, 2020
8283efc
fix close points mouseover/out trigger
lucafalasco Feb 26, 2020
bc65c85
fix import order and package version
lucafalasco Mar 2, 2020
f1fb498
add more test data
lucafalasco Mar 3, 2020
5aec426
add crosshair cursor when ruler is active to better select data points
lucafalasco Mar 3, 2020
3718884
refactor, more docs
lucafalasco Mar 3, 2020
3dcd533
fix axis tooltip baseline
lucafalasco Mar 3, 2020
178ea29
minor refactor
lucafalasco Mar 3, 2020
192e273
change ruler color
lucafalasco Mar 12, 2020
ab66a64
a11y bubble
lucafalasco Mar 13, 2020
b96da08
only show ruler for continuous domain - wip
lucafalasco Mar 13, 2020
7121b63
add support for top axis
lucafalasco Mar 16, 2020
11589b6
no need to support discrete scales anymore
lucafalasco Mar 16, 2020
00d44af
make sure tooltip does not go out of axis bbox
lucafalasco Mar 18, 2020
ae93c80
remove axis tooltip
lucafalasco Apr 1, 2020
7e46f2b
only show ruler if there's match with data points
lucafalasco Apr 1, 2020
6f3ffb9
only show ruler on timescale linechart
lucafalasco Apr 1, 2020
fe6457a
add filter for items getting null values at times, breaking tooltip
lucafalasco Apr 3, 2020
c1edff4
refactor
lucafalasco Apr 3, 2020
c190a2b
fix(core): update import path carbon-components
natashadecoste Mar 30, 2020
b576875
v0.30.7
carbon-bot Mar 31, 2020
a643552
Merge pull request #529 from theiliad/tabular-data-format
theiliad Apr 3, 2020
ff8b0aa
v0.30.8
carbon-bot Apr 3, 2020
7214aa5
Merge branch 'master' into tooltip-enh
lucafalasco Apr 7, 2020
0247757
Apply suggestions from code review
lucafalasco Apr 15, 2020
63687f9
applied suggestions from review
lucafalasco Apr 15, 2020
ff48ac2
fix ruler css
lucafalasco Apr 15, 2020
db3d1f0
Update packages/core/src/components/axes/ruler.ts
lucafalasco Apr 21, 2020
fc4ff2a
Update packages/core/src/styles/components/_ruler.scss
lucafalasco Apr 21, 2020
b4d5bf3
Update packages/core/src/components/axes/ruler.ts
lucafalasco Apr 21, 2020
ee1b29a
add suggestions from review
lucafalasco Apr 21, 2020
d63e6a4
Update packages/core/demo/data/line.ts
lucafalasco Apr 23, 2020
7ef4e63
Apply suggestions from code review
lucafalasco Apr 23, 2020
00c55e3
enable ruler on bubble and scatter
lucafalasco Apr 23, 2020
2095c04
disable mouse listeners on grid
lucafalasco Apr 23, 2020
7a17802
enable ruler for every scale type
lucafalasco Apr 24, 2020
ced86b4
fix ruler on linear scales, refactor matches logic
lucafalasco Apr 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions packages/core/demo/data/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,11 @@ let allDemoGroups = [
chartType: chartTypes.LineChart,
isDemoExample: true
},
{
options: lineDemos.lineTimeSeriesDenseOptions,
data: lineDemos.lineTimeSeriesDenseData,
chartType: chartTypes.LineChart
},
{
options: lineDemos.lineOptions,
data: lineDemos.lineData,
Expand Down
63 changes: 63 additions & 0 deletions packages/core/demo/data/line.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,69 @@ export const lineTimeSeriesOptions = {
curve: "curveMonotoneX"
};

export const lineTimeSeriesDenseData = [
{ group: "Dataset 1", date: new Date(2019, 0, 1), value: 10000 },
{ group: "Dataset 1", date: new Date(2019, 0, 1, 5), value: 12000 },
{ group: "Dataset 1", date: new Date(2019, 0, 1, 10), value: 14000 },
{ group: "Dataset 1", date: new Date(2019, 0, 2), value: 25000 },
{ group: "Dataset 1", date: new Date(2019, 0, 2, 2), value: 26000 },
{ group: "Dataset 1", date: new Date(2019, 0, 3), value: 10000 },
{ group: "Dataset 1", date: new Date(2019, 0, 3, 5), value: 10000 },
{ group: "Dataset 1", date: new Date(2019, 0, 3, 10), value: 12000 },
{ group: "Dataset 1", date: new Date(2019, 0, 5), value: 45000 },
{ group: "Dataset 1", date: new Date(2019, 0, 7), value: 49000 },
{ group: "Dataset 1", date: new Date(2019, 0, 7, 15), value: 45000 },
{ group: "Dataset 1", date: new Date(2019, 0, 9), value: 50000 },
{ group: "Dataset 1", date: new Date(2019, 0, 9, 5), value: 52000 },
{ group: "Dataset 1", date: new Date(2019, 0, 9, 15), value: 55000 },
{ group: "Dataset 1", date: new Date(2019, 0, 10), value: 50000 },
{ group: "Dataset 1", date: new Date(2019, 0, 12), value: 65000 },
{ group: "Dataset 1", date: new Date(2019, 0, 13), value: 80000 },
{ group: "Dataset 1", date: new Date(2019, 0, 14, 10), value: 85000 },
{ group: "Dataset 1", date: new Date(2019, 0, 15, 7), value: 90000 },
{ group: "Dataset 1", date: new Date(2019, 0, 15, 18), value: 70000 },
{ group: "Dataset 2", date: new Date(2019, 0, 1), value: 20000 },
{ group: "Dataset 2", date: new Date(2019, 0, 1, 3), value: 22000 },
{ group: "Dataset 2", date: new Date(2019, 0, 1, 16), value: 24000 },
{ group: "Dataset 2", date: new Date(2019, 0, 2), value: 35000 },
{ group: "Dataset 2", date: new Date(2019, 0, 2, 7), value: 36000 },
{ group: "Dataset 2", date: new Date(2019, 0, 3), value: 20000 },
{ group: "Dataset 2", date: new Date(2019, 0, 3, 6), value: 20000 },
{ group: "Dataset 2", date: new Date(2019, 0, 3, 18), value: 22000 },
{ group: "Dataset 2", date: new Date(2019, 0, 5), value: 62000 },
{ group: "Dataset 2", date: new Date(2019, 0, 6), value: 52000 },
{ group: "Dataset 2", date: new Date(2019, 0, 7), value: 52000 },
{ group: "Dataset 2", date: new Date(2019, 0, 7, 15), value: 52000 },
{ group: "Dataset 2", date: new Date(2019, 0, 7), value: 52000 },
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
{ group: "Dataset 2", date: new Date(2019, 0, 7, 15), value: 52000 },
{ group: "Dataset 2", date: new Date(2019, 0, 9), value: 60000 },
{ group: "Dataset 2", date: new Date(2019, 0, 9, 5), value: 62000 },
{ group: "Dataset 2", date: new Date(2019, 0, 9, 10), value: 62000 },
{ group: "Dataset 2", date: new Date(2019, 0, 12), value: 65000 },
{ group: "Dataset 2", date: new Date(2019, 0, 14), value: 40000 },
{ group: "Dataset 2", date: new Date(2019, 0, 15, 5), value: 45000 },
{ group: "Dataset 2", date: new Date(2019, 0, 15, 10), value: 35000 },
{ group: "Dataset 2", date: new Date(2019, 0, 15, 18), value: 30000 }
];


export const lineTimeSeriesDenseOptions = {
title: "Line (dense time series)",
axes: {
bottom: {
title: "2019 Annual Sales Figures",
mapsTo: "date",
scaleType: "time"
},
left: {
mapsTo: "value",
title: "Conversion rate",
scaleType: "linear"
}
},
curve: "curveMonotoneX"
};

export const lineTimeSeriesDataRotatedTicks = [
{ group: "Dataset 1", date: new Date(2019, 11, 30), value: 32100 },
{ group: "Dataset 1", date: new Date(2019, 11, 31), value: 23500 },
Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/charts/line.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Tools } from "../tools";
import {
Grid,
Line,
Ruler,
Scatter,
TwoDimensionalAxes,
// the imports below are needed because of typescript bug (error TS4029)
Expand Down Expand Up @@ -42,6 +43,7 @@ export class LineChart extends AxisChart {
const graphFrameComponents = [
new TwoDimensionalAxes(this.model, this.services),
new Grid(this.model, this.services),
new Ruler(this.model, this.services),
new Line(this.model, this.services),
new Scatter(this.model, this.services)
];
Expand Down
187 changes: 187 additions & 0 deletions packages/core/src/components/axes/ruler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
// Internal Imports
import { Component } from "../component";
import { DOMUtils } from "../../services";
import { TooltipTypes, ScaleTypes } from "../../interfaces";
import { Tools } from "../../tools";

// D3 Imports
import { mouse, Selection } from "d3-selection";

type GenericSvgSelection = Selection<SVGElement, any, SVGElement, any>;

const THRESHOLD = 5;

/** check if x is inside threshold area extents */
function pointIsWithinThreshold(dx: number, x: number) {
return dx > x - THRESHOLD && dx < x + THRESHOLD;
}

export class Ruler extends Component {
type = "ruler";
backdrop: GenericSvgSelection;
hoveredElements: GenericSvgSelection;

render() {
const domainAxisPosition = this.services.cartesianScales.getDomainAxisPosition();
const domainScaleType = this.services.cartesianScales.getScaleTypeByPosition(
domainAxisPosition
);
const isTimeSeries = domainScaleType === ScaleTypes.TIME;

// if scale type is not timeSeries do not show rule
if (!isTimeSeries) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about other linear scales?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We decided to only enable the ruler for time scales at the moment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's stopping us from enabling it for all linear scales?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea we suggested when we talked about that was to only expect time scales to be used on LineCharts as it's the only one that makes sense with that type of chart


this.drawBackdrop();
this.addBackdropEventListeners();
}

showRuler([x, y]: [number, number]) {
const svg = this.parent;
const ruler = DOMUtils.appendOrSelect(svg, "g.ruler");
const line = DOMUtils.appendOrSelect(ruler, "line.ruler-line");
const dataPoints: GenericSvgSelection = svg.selectAll(
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
"[role=graphics-symbol]"
);
const displayData = this.model.getDisplayData();
const domainScale = this.services.cartesianScales.getDomainScale();
const rangeScale = this.services.cartesianScales.getRangeScale();
const [yScaleEnd, yScaleStart] = rangeScale.range();

const scaledData: number[] = displayData.map((d, i) =>
this.services.cartesianScales.getDomainValue(d, i)
);

/**
* Find matches, reduce is used instead of filter
* to only get elements which belong to the same axis coordinate
*/
const scaledValuesMatches: number[] = scaledData.reduce((accum, currentValue) => {
// store the first element of the accumulator array to compare it with current element being processed
const sampleAccumValue = accum[0];

// if accumulator is not empty and current value is bigger than already existing value in the accumulator, skip current iteration
if (sampleAccumValue && currentValue > sampleAccumValue) {
return accum;
}

// there's a match and currentValue is either less then or equal to already stored values
if (pointIsWithinThreshold(currentValue, x)) {
if (sampleAccumValue && currentValue < sampleAccumValue) {
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
// there's a closer data point in the threshold area, so reinstantiate array
accum = [currentValue];
} else {
// currentValue is equal to already stored values, there's another match on the same coordinate
accum.push(currentValue);
}
}

return accum;
}, []);

// some data point match
if (scaledValuesMatches.length > 0) {
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
const sampleMatch = scaledValuesMatches[0];

const highlightItems = this.services.cartesianScales.getDataFromDomain(
domainScale.invert(sampleMatch)
).filter(d => d.value);

const hoveredElements = dataPoints.filter((d, i) =>
scaledValuesMatches.includes(
Number(this.services.cartesianScales.getDomainValue(d))
)
);
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved

/** if we pass from a trigger area to another one
* mouseout on previous elements won't get dispatched
* so we need to do it manually
*/
if (
this.hoveredElements &&
this.hoveredElements.size() > 0 &&
!Tools.isEqual(this.hoveredElements, hoveredElements)
) {
this.hideRuler();
}

hoveredElements.dispatch("mouseover");

// set current hovered elements
this.hoveredElements = hoveredElements;

this.services.events.dispatchEvent("show-tooltip", {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

hoveredElement: line,
multidata: highlightItems,
type: TooltipTypes.GRIDLINE
});

ruler.attr("opacity", 1);

// line snaps to matching point
line.attr("y1", yScaleStart)
.attr("y2", yScaleEnd)
.attr("x1", sampleMatch)
.attr("x2", sampleMatch);
} else {
ruler.attr("opacity", 0);
dataPoints.dispatch("mouseout");
}
}

hideRuler() {
const svg = this.parent;
const ruler = DOMUtils.appendOrSelect(svg, "g.ruler");
const dataPoints = svg.selectAll("[role=graphics-symbol]");

dataPoints.dispatch("mouseout");
ruler.attr("opacity", 0);
}

/**
* Adds the listener on the X grid to trigger multiple point tooltips along the x axis.
*/
addBackdropEventListeners() {
const self = this;

this.backdrop
.on("mousemove mouseover", function() {
const chartContainer = self.services.domUtils.getMainSVG();
const pos = mouse(chartContainer);

self.showRuler(pos);
})
.on("mouseout", function() {
self.hideRuler();
self.services.events.dispatchEvent("hide-tooltip", {});
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
});
}

drawBackdrop() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a backdrop?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is needed to attach mouse listeners

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that means we can get rid of the grid.ts event listeners for tooltip...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we discussed this and while we don't think it would be good to remove listeners from grid.ts as it would affect other charts too (where ruler does not exist), what we can do is add the possibility to pass a boolean option to the Grid class constructor to disable the event listeners for specific cases (right now only when ruler is active, which means only on the linechart)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to recap after latest discussion, I'm now going to disable mouse listeners on grid and enable ruler interaction for every other chart and scale type 😅

const svg = this.parent;

const domainScale = this.services.cartesianScales.getDomainScale();
const rangeScale = this.services.cartesianScales.getRangeScale();

const [xScaleStart, xScaleEnd] = domainScale.range();
const [yScaleEnd, yScaleStart] = rangeScale.range();

// Get height from the grid
this.backdrop = DOMUtils.appendOrSelect(svg, "svg.chart-grid-backdrop");
const backdropRect = DOMUtils.appendOrSelect(
this.backdrop,
"rect.chart-grid-backdrop"
);

this.backdrop
.merge(backdropRect)
.attr("x", xScaleStart)
.attr("y", yScaleStart)
.attr("width", xScaleEnd - xScaleStart)
.attr("height", yScaleEnd - yScaleStart)
.lower();

backdropRect.attr("width", "100%").attr("height", "100%");
}
}
2 changes: 2 additions & 0 deletions packages/core/src/components/graphs/bubble.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Internal Imports
import { Scatter } from "./scatter";
import { DOMUtils } from "../../services";
import { Roles } from "../../interfaces";

// D3 Imports
import { Selection } from "d3-selection";
Expand Down Expand Up @@ -39,6 +40,7 @@ export class Bubble extends Scatter {

selection.raise()
.classed("dot", true)
.attr("role", Roles.GRAPHICS_SYMBOL)
.attr("cx", (d, i) => this.services.cartesianScales.getDomainValue(d, i))
.transition(this.services.transitions.getTransition("bubble-update-enter", animate))
.attr("cy", (d, i) => this.services.cartesianScales.getRangeValue(d, i))
Expand Down
1 change: 1 addition & 0 deletions packages/core/src/components/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ export * from "./layout/layout";
export * from "./axes/two-dimensional-axes";
export * from "./axes/axis";
export * from "./axes/grid";
export * from "./axes/ruler";
export * from "./axes/zero-line";
2 changes: 1 addition & 1 deletion packages/core/src/configuration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ export const baseTooltip: TooltipOptions = {
export const axisChartTooltip: AxisTooltipOptions = Tools.merge({}, baseTooltip, {
gridline: {
enabled: true,
threshold: 0.25
threshold: 0.02
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current threshold I see on any datapoints seems to be too small. can we increase that please?

Copy link
Contributor Author

@lucafalasco lucafalasco Apr 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the ruler threshold or the gridline threshold? The reason we made both the ruler threshold and the grid threshold values so small is to prevent overlapping trigger events on datapoints which are really close to each other.

}
} as AxisTooltipOptions);

Expand Down
20 changes: 20 additions & 0 deletions packages/core/src/styles/components/_ruler.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
.#{$prefix}--#{$charts-prefix}--ruler {
line.ruler-line {
@if $ui-background == map-get($carbon--theme--g90, 'ui-background') {
stroke: #fff;
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
} @else if $ui-background == map-get($carbon--theme--g100, 'ui-background') {
stroke: #fff;
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
} @else {
stroke: #000;
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
}
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
stroke-width: 1px;
stroke-dasharray: 2;
pointer-events: none;
}

text.axis-tooltip-text {
fill: white;
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
dominant-baseline: middle;
text-anchor: middle;
}
}
1 change: 1 addition & 0 deletions packages/core/src/styles/components/index.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
@import "./axis";
@import "./callouts";
@import "./grid";
@import "./ruler";
@import "./zero-line";
@import "./layout";
@import "./legend";
Expand Down
3 changes: 3 additions & 0 deletions packages/core/src/tools.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
cloneDeep as lodashCloneDeep,
uniq as lodashUnique,
clamp as lodashClamp,
isEqual as lodashIsEqual,
flatten as lodashFlaten,
lucafalasco marked this conversation as resolved.
Show resolved Hide resolved
// the imports below are needed because of typescript bug (error TS4029)
Cancelable,
DebounceSettings
Expand All @@ -24,6 +26,7 @@ export namespace Tools {
export const merge = lodashMerge;
export const removeArrayDuplicates = lodashUnique;
export const clamp = lodashClamp;
export const isEqual = lodashIsEqual;

/**
* Returns default chart options merged with provided options,
Expand Down