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.

Sign up for GitHub

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/box plot labels #17579

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Feat/box plot labels #17579

Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
677c4fd
feat: added labels to box plot
konrad-amtenbrink Aug 26, 2022
9df531d
refactor: refactored naming and label creation
konrad-amtenbrink Aug 26, 2022
7732ae1
Merge branch 'apache:master' into feat/box-plot-labels
konrad-amtenbrink Aug 26, 2022
d94f926
fix: fixed linting + added tests
konrad-amtenbrink Sep 7, 2022
70e835b
Merge branch 'feat/box-plot-labels' of github.com:konrad-amtenbrink/e…
konrad-amtenbrink Sep 7, 2022
4fffef0
feat: add option to configure box plot labels
konrad-amtenbrink Sep 8, 2022
fb1267f
refactor: moved setBoxLabels to labelUtil
konrad-amtenbrink Oct 7, 2022
fa891fe
Merge branch 'master' into feat/box-plot-labels
konrad-amtenbrink Oct 7, 2022
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
60 changes: 53 additions & 7 deletions src/chart/boxplot/BoxplotView.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import ExtensionAPI from '../../core/ExtensionAPI';
import SeriesData from '../../data/SeriesData';
import { BoxplotItemLayout } from './boxplotLayout';
import { saveOldStyle } from '../../animation/basicTransition';
import { ViewRootGroup } from '../../util/types';

class BoxplotView extends ChartView {
static type = 'boxplot';
Expand All @@ -52,7 +53,7 @@ class BoxplotView extends ChartView {
.add(function (newIdx) {
if (data.hasValue(newIdx)) {
const itemLayout = data.getItemLayout(newIdx) as BoxplotItemLayout;
const symbolEl = createNormalBox(itemLayout, data, newIdx, constDim, true);
const symbolEl = createNormalBox(itemLayout, data, newIdx, constDim, group, true);
data.setItemGraphicEl(newIdx, symbolEl);
group.add(symbolEl);
}
Expand All @@ -68,11 +69,11 @@ class BoxplotView extends ChartView {

const itemLayout = data.getItemLayout(newIdx) as BoxplotItemLayout;
if (!symbolEl) {
symbolEl = createNormalBox(itemLayout, data, newIdx, constDim);
symbolEl = createNormalBox(itemLayout, data, newIdx, constDim, group);
}
else {
saveOldStyle(symbolEl);
updateNormalBoxData(itemLayout, symbolEl, data, newIdx);
updateNormalBoxData(itemLayout, symbolEl, data, newIdx, group);
}

group.add(symbolEl);
Expand Down Expand Up @@ -144,7 +145,8 @@ function createNormalBox(
data: SeriesData,
dataIndex: number,
constDim: number,
isInit?: boolean
group: ViewRootGroup,
isInit?: boolean,
) {
const ends = itemLayout.ends;

Expand All @@ -156,7 +158,7 @@ function createNormalBox(
}
});

updateNormalBoxData(itemLayout, el, data, dataIndex, isInit);
updateNormalBoxData(itemLayout, el, data, dataIndex, group, isInit);

return el;
}
Expand All @@ -166,7 +168,8 @@ function updateNormalBoxData(
el: BoxPath,
data: SeriesData,
dataIndex: number,
isInit?: boolean
group: ViewRootGroup,
isInit?: boolean,
) {
const seriesModel = data.hostModel;
const updateMethod = graphic[isInit ? 'initProps' : 'updateProps'];
Expand All @@ -180,17 +183,60 @@ function updateNormalBoxData(

el.useStyle(data.getItemVisual(dataIndex, 'style'));
el.style.strokeNoScale = true;

el.z2 = 100;

const itemModel = data.getItemModel<BoxplotDataItemOption>(dataIndex);
const emphasisModel = itemModel.getModel('emphasis');

if (seriesModel.option.label.show) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't access the attributes using seriesModel.option. Please use const labelModel = seriesModel.get('label'); const show = labelModel.get('show'); instead.

Please also change seriesModel.option in other places.

const formattedLabels =
((seriesModel as BoxplotSeriesModel).getRawValue(dataIndex) as number[])
.splice(1).map((value: number) => seriesModel.option.label.formatter(value).toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably not create a Text element and do all the stuffs by yourself. Instead, setLabelStyle should be called. See how this is done with line series. But in boxplot's case, there are multiple labels so you need to figure out how this should be done, but generally speaking, setLabelStyle should be called most likely.

Copy link
Author

Choose a reason for hiding this comment

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

I would not add it to setLabelStyle as it is super specific code which only applies to the bar chart, so it doesn't really make sense in my opinion to add it a general util method. However, I agree that it should live in the labelStyle util. So I would vote to move it there and refactor it so it does not create a Text element manually. What do you think?

Copy link

Choose a reason for hiding this comment

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

@Ovilia any news on this? Is the proposed solution good enough?


setBoxLabels(itemLayout.ends, formattedLabels, seriesModel.option.label, group);
}

setStatesStylesFromModel(el, itemModel);

toggleHoverEmphasis(el, emphasisModel.get('focus'), emphasisModel.get('blurScope'), emphasisModel.get('disabled'));
}

function setBoxLabels(boxItemPositions: number[][], formattedLabels: Array<string>, labelOption: any, group: ViewRootGroup) {
// get sorted and unique y positions of box items
const uniqueYPositions = Array.from(new Set(boxItemPositions.map((pos: number[]) => pos[1])));
uniqueYPositions.sort(function(a: number, b: number) {
return a - b;
});

boxItemPositions.sort(function(a: number[], b: number[]) {
return a[0] - b[0];
});

// get alternating y positions for labels to avoid overlap
const uniqueAlternatingPositions = uniqueYPositions.map((posY: number, ind: number) => {
const matchingPositions = boxItemPositions.filter((orgPos: number[]) => orgPos[1] == posY);
const index = (ind % 2 == 0) ? 0 : matchingPositions.length - 1;
return matchingPositions[index];
});

// create labels and add them to their respective positions
formattedLabels.forEach((labelText: string, ind: number) => {
const label = new graphic.Text({
style: {
text: labelText
},
z2: 1000
});
const defaultOffset = 5;
const defaultXOffset = (ind % 2) === 0 ? (- (label.getBoundingRect().width + defaultOffset)) : defaultOffset;
const defaultYOffset = - defaultOffset;
const customOffset = labelOption.offset ? labelOption.offset : [0, 0];
label.x = uniqueAlternatingPositions[ind][0] + defaultXOffset + customOffset[0],
label.y = uniqueAlternatingPositions[ind][1] + defaultYOffset + customOffset[1],
group.add(label);
});
}

function transInit(points: number[][], dim: number, itemLayout: BoxplotItemLayout) {
return zrUtil.map(points, function (point) {
point = point.slice();
Expand Down