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

Fix data labels too dense with large single series #42985

Merged

Conversation

JesseSDevaney
Copy link
Contributor

@JesseSDevaney JesseSDevaney commented May 21, 2024

@JesseSDevaney JesseSDevaney added backport Automatically create PR on current release branch on merge .Team/DashViz Dashboard and Viz team labels May 21, 2024
@JesseSDevaney JesseSDevaney requested review from alxnddr and a team May 21, 2024 21:45
@JesseSDevaney JesseSDevaney self-assigned this May 21, 2024
Copy link

github-actions bot commented May 21, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff 2c1333e...60f4344.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts
frontend/src/metabase/visualizations/echarts/cartesian/model/index.ts
frontend/src/metabase/visualizations/echarts/cartesian/model/series.ts
frontend/src/metabase/visualizations/echarts/cartesian/model/types.ts
frontend/src/metabase/visualizations/echarts/cartesian/option/axis.ts
frontend/src/metabase/visualizations/echarts/cartesian/option/goal-line.ts
frontend/src/metabase/visualizations/echarts/cartesian/option/series.ts
frontend/src/metabase/visualizations/echarts/cartesian/option/trend-line.ts
frontend/src/metabase/visualizations/echarts/cartesian/option/utils.ts
frontend/src/metabase/visualizations/echarts/cartesian/scatter/model/index.ts
frontend/src/metabase/visualizations/echarts/cartesian/timeline-events/model.ts
frontend/src/metabase/visualizations/echarts/cartesian/waterfall/model/index.ts
frontend/src/metabase/visualizations/echarts/cartesian/waterfall/option/index.ts
frontend/src/metabase/visualizations/visualizations/CartesianChart/events.ts
frontend/src/metabase/visualizations/visualizations/CartesianChart/use-chart-debug.ts
frontend/src/metabase/visualizations/visualizations/CartesianChart/use-chart-events.ts
frontend/src/metabase/visualizations/visualizations/CartesianChart/use-models-and-option.ts
frontend/src/metabase/visualizations/visualizations/CartesianChart/utils.ts

Copy link

replay-io bot commented May 21, 2024

Status Complete ↗︎
Commit 60f4344
Results
⚠️ 4 Flaky
2580 Passed

Copy link
Member

@alxnddr alxnddr left a comment

Choose a reason for hiding this comment

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

I think we need to tweak the logic based on chart width, data points density, and maybe data labels widths becuase it looks too rare:
Screenshot 2024-05-22 at 3 06 11 PM

@JesseSDevaney JesseSDevaney force-pushed the fix-40196/data-labels-too-dense-with-large-single-series branch from a0a5c8e to 35a8b6d Compare May 30, 2024 20:15
@JesseSDevaney JesseSDevaney force-pushed the fix-40196/data-labels-too-dense-with-large-single-series branch from 0f8331d to 9ece4d5 Compare May 30, 2024 20:24
@JesseSDevaney JesseSDevaney force-pushed the fix-40196/data-labels-too-dense-with-large-single-series branch from 9ece4d5 to 9d1c2c2 Compare May 30, 2024 20:26
Copy link
Member

@alxnddr alxnddr left a comment

Choose a reason for hiding this comment

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

Looks good, I left two minor comments, please have a look before merging

index a75f606..57bec85 100644
--- a/node_modules/echarts/lib/label/labelLayoutHelper.js
+++ b/node_modules/echarts/lib/label/labelLayoutHelper.js
@@ -256,6 +256,13 @@ export function hideOverlap(labelList) {
Copy link
Member

Choose a reason for hiding this comment

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

It would be possible to get this behavior without patching ECharts by using labelLayout and shifting empty labels to one single place outside of the chart but it would be quite hackish. So I agree that ECharts should ignore empty labels when computing layout

@JesseSDevaney JesseSDevaney enabled auto-merge (squash) May 31, 2024 18:00
@JesseSDevaney JesseSDevaney enabled auto-merge (squash) May 31, 2024 18:01
@JesseSDevaney JesseSDevaney merged commit 72443c6 into master Jun 1, 2024
109 checks passed
@JesseSDevaney JesseSDevaney deleted the fix-40196/data-labels-too-dense-with-large-single-series branch June 1, 2024 06:05
Copy link

github-actions bot commented Jun 1, 2024

@JesseSDevaney Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

github-automation-metabase pushed a commit that referenced this pull request Jun 1, 2024
* remove unused variable

* conditionally render labels sparsely if large amount of data points

* update loki snapshots

* refactor data label formatter conditions

* setup intelligent label plotting

* handle the case when there are no labels

* add sparse data label rendering to bar charts

* do not compute if no labels need to be shown or hidden

* add sparse data labeling to waterfall charts

* adjustment for large series waterfall labels

* revert change

* refactor name

* add sparse labels to stacked bar and area

* improve scaleFactor

* fix type errors

* remove unused labelFormatter

* update snapshots

* patch ECharts

- labels with no text, i.e. "", were still being considered as plot-able labels which was causing labels with real text "asdfec.." to be hidden because of overlap.
- This patch makes it so that labels with no text, i.e. "", are not considered to be candidates for overlap checking.

* update loki snapshots

* only loop over dataset once for getWaterfallChartDataDensity

* improve performance of chart data density calculations

* update loki snapshots

* update E2E spec

* update E2E spec

* increase cartesian label density allotment

* update loki snapshots

* update loki snapshots

* comment out node_modules cache since it cannot be skipped by commit and is breaking CI

* fix type errors

* Reset node modules if patches are changed

* refactor type naming

---------

Co-authored-by: Uladzimir Havenchyk <uladzimir.dev@gmail.com>
Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.com>
JesseSDevaney added a commit that referenced this pull request Jun 1, 2024
* remove unused variable

* conditionally render labels sparsely if large amount of data points

* update loki snapshots

* refactor data label formatter conditions

* setup intelligent label plotting

* handle the case when there are no labels

* add sparse data label rendering to bar charts

* do not compute if no labels need to be shown or hidden

* add sparse data labeling to waterfall charts

* adjustment for large series waterfall labels

* revert change

* refactor name

* add sparse labels to stacked bar and area

* improve scaleFactor

* fix type errors

* remove unused labelFormatter

* update snapshots

* patch ECharts

- labels with no text, i.e. "", were still being considered as plot-able labels which was causing labels with real text "asdfec.." to be hidden because of overlap.
- This patch makes it so that labels with no text, i.e. "", are not considered to be candidates for overlap checking.

* update loki snapshots

* only loop over dataset once for getWaterfallChartDataDensity

* improve performance of chart data density calculations

* update loki snapshots

* update E2E spec

* update E2E spec

* increase cartesian label density allotment

* update loki snapshots

* update loki snapshots

* comment out node_modules cache since it cannot be skipped by commit and is breaking CI

* fix type errors

* Reset node modules if patches are changed

* refactor type naming

---------

Co-authored-by: Jesse Devaney <22608765+JesseSDevaney@users.noreply.github.com>
Co-authored-by: Uladzimir Havenchyk <uladzimir.dev@gmail.com>
Co-authored-by: Uladzimir Havenchyk <125459446+uladzimirdev@users.noreply.github.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
Projects
None yet
5 participants