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

Elevate selected filter field values #40055

Merged
merged 6 commits into from Mar 13, 2024
Merged

Elevate selected filter field values #40055

merged 6 commits into from Mar 13, 2024

Conversation

ranquild
Copy link
Contributor

@ranquild ranquild commented Mar 13, 2024

Product doc https://www.notion.so/metabase/Show-the-selected-filter-values-on-top-of-multi-select-list-6e3f2e1880dc40bc8c50170236ca0aaa

Elevate selected field values on initial render when rendering them as a list of checkboxes.

How to verify:

  • New -> Question -> Products
  • Add a filter Category -> Gizmo
  • Click on the added filter
  • See that the Gizmo is at the top of the list
  • Try checking and unchecking options. Gizmo stays at the same position, other options also don't jump.
Screenshot 2024-03-13 at 13 37 01 Screenshot 2024-03-13 at 13 37 07

@ranquild ranquild self-assigned this Mar 13, 2024
@ranquild ranquild added the backport Automatically create PR on current release branch on merge label Mar 13, 2024
@ranquild ranquild marked this pull request as ready for review March 13, 2024 11:38
@ranquild ranquild requested review from a team, cdeweyx and mngr March 13, 2024 11:38
@@ -242,6 +242,66 @@ describe("StringFilterValuePicker", () => {
expect(onChange).toHaveBeenCalledWith(["t", "p"]);
});

it("should elevate selected field values on initial render", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All tests here with custom remapping to handle the most advance case.

Copy link

replay-io bot commented Mar 13, 2024

Status Complete ↗︎
Commit 4aed83f
Results
⚠️ 4 Flaky
2357 Passed

query={query}
stageIndex={stageIndex}
column={column}
values={["p", "c"]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also make sure that unchecking values doesn't move the items.
This can be a separate test case, or this one:

Suggested change
values={["p", "c"]}
values={["c"]}

plus expect(checkboxes[0]).not.toBeChecked();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test

@@ -57,7 +57,7 @@ async function setupStringPicker({
setupFieldSearchValuesEndpoints(result.field_id, value, result.values);
});

renderWithProviders(
const { rerender } = renderWithProviders(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this case be handled too?

2024-03-13.18-52-36.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, great catch!

@kamilmielnik kamilmielnik requested a review from a team March 13, 2024 11:55
@kamilmielnik kamilmielnik requested a review from a team March 13, 2024 12:16
@ranquild ranquild merged commit 1a232f0 into master Mar 13, 2024
109 checks passed
@ranquild ranquild deleted the selected-list-values branch March 13, 2024 13:09
Copy link

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

github-actions bot pushed a commit that referenced this pull request Mar 13, 2024
@ranquild ranquild added this to the 0.49 milestone Mar 13, 2024
metabase-bot bot added a commit that referenced this pull request Mar 13, 2024
Co-authored-by: Alexander Polyankin <alexander.polyankin@metabase.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/QueryingComponents
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants