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 pie rows settings include filtered out values #47879

Merged
merged 8 commits into from
Sep 13, 2024

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented Sep 11, 2024

Closes #47867

Description

After editing pie rows settings, if dataset returned fewer rows we still showed previously existing rows in the settings sidebar. This PR filters them out.

How to verify

  • Create a Pie chart, reorder rows
  • Apply filter or row limit to remove some rows from the dataset
  • Ensure the settings sidebar shows rows that only exist in the dataset

Checklist

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

@metabase-bot metabase-bot bot added the .Team/DashViz Dashboard and Viz team label Sep 11, 2024
@alxnddr alxnddr added the no-backport Do not backport this PR to any branch label Sep 11, 2024
Copy link
Contributor

@EmmadUsmani EmmadUsmani left a comment

Choose a reason for hiding this comment

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

The problem with this approach is that when you edit pie.rows and save the question after rows have been removed from the query result, when those rows reappear they will no longer have their settings. This is why we kept track of the removed rows and marked them with hidden: true instead of removing them from the setting. See the example below

Screen.Recording.2024-09-11.at.5.00.13.PM.mov

@alxnddr alxnddr force-pushed the fix-pie-rows-settings-include-filtered-out-values branch from ee09f58 to fb8dad3 Compare September 12, 2024 21:54
@alxnddr
Copy link
Member Author

alxnddr commented Sep 12, 2024

@EmmadUsmani thanks, I fixed that

Copy link
Contributor

@EmmadUsmani EmmadUsmani 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 👍

@alxnddr alxnddr enabled auto-merge (squash) September 13, 2024 03:01
@alxnddr alxnddr merged commit 24235d2 into master Sep 13, 2024
120 of 121 checks passed
@alxnddr alxnddr deleted the fix-pie-rows-settings-include-filtered-out-values branch September 13, 2024 14:32
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slices removed from query result still appear in pie chart settings
2 participants