-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Support stacked bar series labels #42803
Conversation
Codenotify: Notifying subscribers in CODENOTIFY files for diff 246f9b5...96f2cdb.
|
|
4bb1b3b
to
8dea55b
Compare
}; | ||
} | ||
|
||
const labelValue = dataset[dataIndex][seriesDataKey]; |
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.
Should use transformedDataset[ORIGINAL_INDEX_DATA_KEY]
for consistency with buildEChartsStackLabelOptions
, or in buildEChartsStackLabelOptions
just use params.dataIndex
for consistency with this function
8df9c69
to
e689646
Compare
66f2898
to
9c90d3a
Compare
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.
Hi Sasha! Nice work on this overall ❤️
This feels 99% of the way there, but there are still a couple things I think we should address before merging.
- We should probably hide the
Values to show
setting inStack - 100%
WITHOUT a line series because it will not affect the individual labels for stacked series. Or does it? I could not find a space where it was affecting whether some inside labels were displayed or hidden.
- Also, we no longer display the
Auto formatting
setting which still applies to these labels. See above and below for comparison. The setting is carried over but not adjustable inStack - 100%
mode.
(assuming if we still want to display absolute values instead of relative percentages - relative percentages makes more sense to me considering the specified chart type) - (in auto formatting mode) if one series is compact, we should probably compact the labels for the rest of the series. a mix of compact and full feels wonky.
- I also left some individual comments on other areas in the code.
.loki/reference/chrome_laptop_static_viz_ComboChart_Bar_Stacked_Normalized_Series_Labels.png
Outdated
Show resolved
Hide resolved
d711fac
to
8ad8bed
Compare
@JesseSDevaney thanks for the review, I addressed your feedback here, please have a look 8ad8bed |
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.
Sasha, thank you for addressing my comments! ❤️
There are a couple small things left to polish and then we should be good to go
barSeriesLabelsFormattingInfo.forEach( | ||
({ compactFormatter, fullFormatter, dataKey }) => { | ||
formatters[dataKey] = shouldApplyCompactFormattingToBarStack | ||
? compactFormatter | ||
: fullFormatter; | ||
}, | ||
); | ||
} |
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.
Here are the code suggestions based on the above observations and suggestions
additional type imported (here)
RawValueFormatter,
getSeriesLabelsFormattingInfo (here)
I added a type for use in the getSeriesLabelsFormatters
function.
type SeriesLabelFormattingInfo = {
dataKey: DataKey;
fullFormatter: RawValueFormatter;
compactFormatter: RawValueFormatter;
isCompact: boolean;
};
const getSeriesLabelsFormattingInfo = (
seriesModels: SeriesModel[],
dataset: ChartDataset,
settings: ComputedVisualizationSettings,
renderingContext: RenderingContext,
): SeriesLabelFormattingInfo[] => {
getSeriesLabelsFormatters (here)
export const getSeriesLabelsFormatters = (
seriesModels: SeriesModel[],
stackModels: StackModel[],
dataset: ChartDataset,
settings: ComputedVisualizationSettings,
renderingContext: RenderingContext,
): {
formatters: SeriesFormatters;
compactSeriesDataKeys: DataKey[];
} => {
const formatters: SeriesFormatters = {};
const compactSeriesDataKeys: DataKey[] = [];
if (!settings["graph.show_values"]) {
return { formatters, compactSeriesDataKeys };
}
const seriesModelsWithLabels = seriesModels.filter(seriesModel => {
const seriesSettings =
settings.series(seriesModel.legacySeriesSettingsObjectKey) ?? {};
return !!seriesSettings["show_series_values"];
});
// Non-stacked series formatters
const stackedSeriesKeys = new Set(
stackModels.flatMap(stackModel => stackModel.seriesKeys),
);
const nonStackedSeries = seriesModelsWithLabels.filter(
seriesModel => !stackedSeriesKeys.has(seriesModel.dataKey),
);
const seriesLabelFormattingInfo = getSeriesLabelsFormattingInfo(
nonStackedSeries,
dataset,
settings,
renderingContext,
);
// Bar stack series formatters
let barSeriesLabelsFormattingInfo: SeriesLabelFormattingInfo[] = [];
const shouldShowStackedBarSeriesLabels =
settings["graph.show_stack_values"] === "series" ||
settings["graph.show_stack_values"] === "all";
if (shouldShowStackedBarSeriesLabels) {
const barStackSeriesKeys = new Set(
stackModels.find(stackModel => stackModel.display === "bar")
?.seriesKeys ?? [],
);
const barStackSeries = seriesModelsWithLabels.filter(seriesModel =>
barStackSeriesKeys.has(seriesModel.dataKey),
);
barSeriesLabelsFormattingInfo = getSeriesLabelsFormattingInfo(
barStackSeries,
dataset,
settings,
renderingContext,
);
}
const shouldFormattersBeCompact =
seriesLabelFormattingInfo.some(({ isCompact }) => isCompact) ||
barSeriesLabelsFormattingInfo.some(({ isCompact }) => isCompact);
seriesLabelFormattingInfo.forEach(
({ compactFormatter, fullFormatter, dataKey }) => {
formatters[dataKey] = shouldFormattersBeCompact
? compactFormatter
: fullFormatter;
shouldFormattersBeCompact && compactSeriesDataKeys.push(dataKey);
},
);
barSeriesLabelsFormattingInfo.forEach(
({ compactFormatter, fullFormatter, dataKey }) => {
formatters[dataKey] = shouldFormattersBeCompact
? compactFormatter
: fullFormatter;
},
);
return { formatters, compactSeriesDataKeys };
};
295fd34
to
c622048
Compare
* refactor scatter plot to separate code path (#43032) * refactor scatter plot model to separate function * (2/3) refactor scatter plot option to separate function (#43033) * use getScatterPlotOption * use buildEChartsScatterSeries directly * remove hoveredSeriesDataKey from scatter * (3/3) improve chart model types (#43034) * create WaterfallChartModel type * create ScatterPlotModel type * rename BaseCartesianChartModel to CartesianChartModel * simplify chartModel creation in hook * support stacked series values labels (#42803) * drop comment --------- Co-authored-by: Emmad Usmani <emmadusmani@berkeley.edu> Co-authored-by: Aleksandr Lesnenko <alxnddr@users.noreply.github.com> Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
Closes #41980 #13096
Description
Adds the ability to show data labels of individual series on stacked bar charts.
How to verify
Demo
Screen.Recording.2024-05-24.at.9.19.33.PM.mov
Checklist