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

Move series data label formatters from option to the model #42616

Merged
merged 34 commits into from
May 17, 2024

Conversation

JesseSDevaney
Copy link
Contributor

@JesseSDevaney JesseSDevaney commented May 14, 2024


After Changes

2024-05-14.19-52-45.mp4
  • Everything should work the same as before

@alxnddr At :55, it appears there is a new edge case introduced by stacked combo charts with non-stacked/regular lines. The ability to toggle Show values for this series is hidden even though it is still a viable option in this new paradigm. Would you be able to account for this in your combo stacked chart PR?

@JesseSDevaney JesseSDevaney added the .Team/DashViz Dashboard and Viz team label May 14, 2024
@JesseSDevaney JesseSDevaney self-assigned this May 14, 2024
@JesseSDevaney JesseSDevaney added the no-backport Do not backport this PR to any branch label May 14, 2024
- TODO: move stacked data label formatters to the model
@JesseSDevaney JesseSDevaney force-pushed the move-data-label-formatting-to-model branch from da7941c to 952084c Compare May 14, 2024 01:12
@JesseSDevaney JesseSDevaney changed the base branch from master to propagate-combo-stacking-to-areas-bars May 14, 2024 19:34
* propagate combo stacking to area and bar charts

* enable combo stacking for bar/area charts, fix data transform

* fix types, specs

* fix y-axis extents calculations

* remove spec that tested the removed control, update specs, update test data
Base automatically changed from propagate-combo-stacking-to-areas-bars to combo-stacked-chart May 14, 2024 19:40
@JesseSDevaney JesseSDevaney force-pushed the move-data-label-formatting-to-model branch from fa00fc1 to 5225689 Compare May 14, 2024 22:49
@JesseSDevaney JesseSDevaney mentioned this pull request May 14, 2024
1 task
alxnddr and others added 8 commits May 14, 2024 20:25
* propagate combo stacking to area and bar charts

* enable combo stacking for bar/area charts, fix data transform

* fix types, specs

* fix y-axis extents calculations

* remove spec that tested the removed control, update specs, update test data
@JesseSDevaney JesseSDevaney marked this pull request as ready for review May 14, 2024 23:56
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label May 14, 2024
Copy link

github-actions bot commented May 14, 2024

Codenotify: Notifying subscribers in CODENOTIFY files for diff c059690...b261af8.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/echarts/cartesian/chart-measurements/index.ts
frontend/src/metabase/visualizations/echarts/cartesian/model/axis.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/series.ts
frontend/src/metabase/visualizations/echarts/cartesian/utils/formatter.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/lib/settings/graph.js

Copy link

replay-io bot commented May 15, 2024

Status Complete ↗︎
Commit b261af8
Results
⚠️ 1 Flaky
2512 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.

Looks great 🙌

Base automatically changed from combo-stacked-chart to master May 16, 2024 21:40
@JesseSDevaney JesseSDevaney enabled auto-merge (squash) May 17, 2024 00:49
JesseSDevaney and others added 2 commits May 17, 2024 03:19
* refactor function

* remove "graph.show_values" conditional for "graph.label_value_formatting" section

- removed this conditional since this formatting option will now apply to the y-axis as well

* sync axis compact formatting with series/stacked/waterfall compact formatting

* compact y-axis tick values even if no values are shown on data points

* adjust auto compacting length difference

* add stories for compacting y-axis

* Update Loki Snapshots

* update bar-auto-formatting-full

* rename parameter

* revert compact formatting threshold

* rename variable

* fix E2E spec y-axis tick value check

- since tick values can now be compacted, this test had to be updated because it fit within those conditions for compact formatting

* refactor y-axis compact formatting check

* improve padding between y-axis label and tick-values

* update loki visualization files

- since the margin between the y-axis label and y-axis tick values was increased, all of the loki files are being updated

* revert axisNameMargin change

* fix padding

* update loki snapshots

* refactor getValuesToMeasure

* conditionally test extra y-tick values
@JesseSDevaney JesseSDevaney merged commit c747c17 into master May 17, 2024
106 checks passed
@JesseSDevaney JesseSDevaney deleted the move-data-label-formatting-to-model branch May 17, 2024 07:02
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/DashViz Dashboard and Viz team visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dc.js migration] Data labels auto-compactness should not be recomputed on hovers
2 participants