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

Support stacked bar series labels #42803

Merged
merged 2 commits into from
May 30, 2024
Merged

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented May 16, 2024

Closes #41980 #13096

Description

Adds the ability to show data labels of individual series on stacked bar charts.

How to verify

  • create a stacked bar/combo charts
  • enable showing individual series labels using "Stack values to show"

Demo

Screen.Recording.2024-05-24.at.9.19.33.PM.mov

Checklist

  • Tests have been added/updated to cover changes in this PR

@metabase-bot metabase-bot bot added .Team/DashViz Dashboard and Viz team visual Run Percy visual testing labels May 16, 2024
Copy link

github-actions bot commented May 16, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 246f9b5...96f2cdb.

Notify File(s)
@ranquild frontend/src/metabase/lib/colors/palette.ts

Copy link

replay-io bot commented May 16, 2024

Status Starting
Commit 96f2cdb

@alxnddr alxnddr force-pushed the support-stacked-series-labels branch from 4bb1b3b to 8dea55b Compare May 16, 2024 22:28
@alxnddr alxnddr requested a review from a team May 16, 2024 22:33
@EmmadUsmani EmmadUsmani self-requested a review May 16, 2024 23:13
};
}

const labelValue = dataset[dataIndex][seriesDataKey];
Copy link
Contributor

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

Base automatically changed from feature-41981/provide-compact-numbers-for-y-axis-tick-values to move-data-label-formatting-to-model May 17, 2024 06:19
@alxnddr alxnddr added the no-backport Do not backport this PR to any branch label May 17, 2024
@alxnddr alxnddr force-pushed the support-stacked-series-labels branch from 8df9c69 to e689646 Compare May 17, 2024 06:59
Base automatically changed from move-data-label-formatting-to-model to master May 17, 2024 07:02
@alxnddr alxnddr added backport Automatically create PR on current release branch on merge and removed no-backport Do not backport this PR to any branch labels May 25, 2024
@alxnddr alxnddr requested review from kdoh, cdeweyx and a team May 25, 2024 01:06
@alxnddr alxnddr force-pushed the support-stacked-series-labels branch from 66f2898 to 9c90d3a Compare May 26, 2024 23:51
@alxnddr alxnddr requested a review from EmmadUsmani May 28, 2024 14:54
@cdeweyx cdeweyx modified the milestone: 0.50 May 29, 2024
@JesseSDevaney JesseSDevaney self-requested a review May 29, 2024 18:23
Copy link
Contributor

@JesseSDevaney JesseSDevaney left a 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.


  1. We should probably hide the Values to show setting in Stack - 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.
    image
  2. 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 in Stack - 100% mode.
    image
    (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)
  3. (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.
    image
  4. I also left some individual comments on other areas in the code.

@alxnddr alxnddr force-pushed the support-stacked-series-labels branch from d711fac to 8ad8bed Compare May 30, 2024 02:37
@alxnddr
Copy link
Member Author

alxnddr commented May 30, 2024

@JesseSDevaney thanks for the review, I addressed your feedback here, please have a look 8ad8bed

@alxnddr alxnddr requested review from JesseSDevaney and a team May 30, 2024 02:38
Copy link
Contributor

@JesseSDevaney JesseSDevaney left a 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

Comment on lines 524 to 535
barSeriesLabelsFormattingInfo.forEach(
({ compactFormatter, fullFormatter, dataKey }) => {
formatters[dataKey] = shouldApplyCompactFormattingToBarStack
? compactFormatter
: fullFormatter;
},
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly to the above comment about compact formatting, the stacked bar labels are full formatted but the line is compact formatted. We should probably have either all full or all compact displays. See my comments on the whole file as to the changes that I tested out.

Before
image

After
image

Copy link
Contributor

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 };
};

@alxnddr alxnddr force-pushed the support-stacked-series-labels branch from 295fd34 to c622048 Compare May 30, 2024 18:57
@alxnddr alxnddr enabled auto-merge (squash) May 30, 2024 19:01
@alxnddr alxnddr merged commit 642a704 into master May 30, 2024
109 checks passed
@alxnddr alxnddr deleted the support-stacked-series-labels branch May 30, 2024 20:59
alxnddr added a commit that referenced this pull request May 30, 2024
alxnddr added a commit that referenced this pull request May 31, 2024
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offer option to display all data labels within stacked bar charts
4 participants