Skip to content

feat(Native Filters): Exclude Filter Values - #33054

Merged
geido merged 29 commits into
apache:masterfrom
amaannawab923:task-preset
Apr 17, 2025
Merged

feat(Native Filters): Exclude Filter Values#33054
geido merged 29 commits into
apache:masterfrom
amaannawab923:task-preset

Conversation

@amaannawab923

@amaannawab923 amaannawab923 commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

SUMMARY

This pr modifies the existing Inverse Selection feature , and makes it dynamic , Now if the Inverse Selection is enabled
it will show up a Select besides the filter which has 2 values , Is and Is not
where
IS: IN
IS NOT : NOT IN
and the user can switch between these .

It is backward compatible and will be true for Existing Native Filters with Inverse Selection

screen-capture.2.webm

It works in both Vertical and horizontal filter bar

Screenshot 2025-04-15 at 1 43 07 AM Screenshot 2025-04-15 at 1 43 21 AM

Current Native Filter Control panel untouched with just inverse Selection Checkbox , which is being used to operate the is and is not Select for dynamic control
Screenshot 2025-04-15 at 1 44 24 AM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

To test this functionality:
Edit a dashboard and open the native filters panel.
Click Add/Edit Filters and choose the dataset and column you want to filter on.
Under Filter Settings, scroll down and enable the checkbox:
✅ Inverse Selection
Save the filter.

Now in the dashboard view, you will see a new select dropdown besides the native filter with 2 options
is and is not where
IS: IN
IS NOT : NOT IN

Select a few values and have not in selected
Click Apply and verify that the charts exclude the selected values using the NOT IN logic.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@korbit-ai

korbit-ai Bot commented Apr 9, 2025

Copy link
Copy Markdown

Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.

Your admin can change your review schedule in the Korbit Console

@amaannawab923 amaannawab923 changed the title Exclude Filfter Values Exclude Filter Values Apr 9, 2025

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@pull-request-size pull-request-size Bot added size/L and removed size/M labels Apr 9, 2025
@geido geido changed the title Exclude Filter Values feat(Native Filters): Exclude Filter Values Apr 10, 2025
@github-actions

Copy link
Copy Markdown
Contributor

@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

Comment thread superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx Outdated
Comment on lines +182 to +214
useEffect(() => {
const container = current?.children.item(0);
if (!container) return;

const childrenArray = Array.from(container.children);

const resizeObserver = new ResizeObserver(() => {
recalculateItemWidths();
});

childrenArray.forEach(child => resizeObserver.observe(child));

return () => {
childrenArray.forEach(child => resizeObserver.unobserve(child));
resizeObserver.disconnect();
};
}, [current, items.length]);

// callback to update item widths so that the useLayoutEffect runs whenever
// width of any of the child changes
const recalculateItemWidths = () => {
const container = current?.children.item(0);
if (container) {
const { children } = container;
const childrenArray = Array.from(children);

const currentWidths = childrenArray.map(child => child.getBoundingClientRect().width);

// Update state with new widths
setItemsWidth(currentWidths);
}
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am curious as of why all of this logic came into existence? What is this trying to solve?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Earlier the native filter width was constant at gridUnit * 41 so we needed to calculate the overflowing elements only when a native filter was added or removed... And the UseLayoutEffect hook doesnt run whenever an existing Native filter adds in or loses the Exclude filter values checkbox and also the item widths arent updated whenever we add in or remove the dynamic exclude checkbox

What happens when we remove this code ?
Screenshot 2025-04-10 at 9 36 14 PM

I have added 6 filters which looks perfectly fine because the overflowing are being calculated properly but what happens if i add exclude filters checkbox on 2 of those without these resize observers
Screenshot 2025-04-10 at 9 36 39 PM
The item widths arent updated , the overflowing elements arent recalculated and the 4th one overflows even when it should have gone inside the dropdown

When i add the resize observers back , the calculations re run and the 4th one is pushed inside the dropdown , which is expected

Screenshot 2025-04-10 at 9 39 00 PM

return {
FilterControlContainer: HorizontalFilterControlContainer,
FormItem: HorizontalFormItem,
FormItem: (props: any) => <HorizontalFormItem {...props} showExcludeSelection={showExcludeSelection} />,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there any reasons to pass showExcludeSelection explicitly? It seems this isn't necessary for all other settings, what makes this special?

@amaannawab923 amaannawab923 Apr 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This particular setting is when the FormItem is part of horizontal list but not in overflowing dropdown and not in Vertical list

In this case the Width of this native filter is frozen to gridUnit * 41 px but we need space for the exclude filters checkbox as well so this prop is used as Styled Components props

Comment thread superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx Outdated
<span data-test="exclude-filter-label">
{filterBarOrientation === FilterBarOrientation.Horizontal ? t('Exclude Values') : t('Exclude Filter Values')}
</span>
<Tooltip

@geido geido Apr 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the InfoTooltip could work here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2025-04-10 at 10 55 26 PM

Tried using it .... But its a different svg altogether that doesnt match the shared designs ... The infotooltip svg has a single path whereas the one that is required by designs belongs to the chart control panel which has 2 paths and is able to take the correct color

@amaannawab923
amaannawab923 marked this pull request as ready for review April 10, 2025 19:39
@github-actions

Copy link
Copy Markdown
Contributor

@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

Comment thread superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx Outdated
@github-actions

Copy link
Copy Markdown
Contributor

@geido Ephemeral environment spinning up at http://34.212.99.121:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@github-actions

Copy link
Copy Markdown
Contributor

@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@github-actions

Copy link
Copy Markdown
Contributor

@geido Ephemeral environment spinning up at http://44.245.202.222:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@geido

geido commented Apr 16, 2025

Copy link
Copy Markdown
Member

I see this behavior where the item fits the horizontal bar. When I resize the window down it correctly goes inside "More filters" but when I expand it back again, it is not showing back in the bar outside of the "More filters" as it should. Is this happening on master too or was it introduced in this PR?

Also, you have some Cypress failures that seem related.

USA.Births.Names.1.mp4

@amaannawab923

amaannawab923 commented Apr 16, 2025

Copy link
Copy Markdown
Contributor Author

I see this behavior where the item fits the horizontal bar. When I resize the window down it correctly goes inside "More filters" but when I expand it back again, it is not showing back in the bar outside of the "More filters" as it should. Is this happening on master too or was it introduced in this PR?

Also, you have some Cypress failures that seem related.

USA.Births.Names.1.mp4

@geido , I was able to fix the cypress test , it was due to including current in the dependancy array of useEffect which was causing a bit re renders and interrupting with cypress operations ... Removing that from the dependancy array fixed the cypress tests
Now the cypress tests are passing as seen below in the attatched ss ,
Screenshot 2025-04-16 at 10 34 20 PM

However , I was not able to reproduce the bug from video from the previous comment on the Ephemeral Environment , Can you please give info about the browser that you are using as seen in the video below
screen-capture (3).webm

And it shouldnt induce a bug like this as im only attatching resizeobservers on top of elements and updating item widths , However i will try testing on different browsers as well

@github-actions

Copy link
Copy Markdown
Contributor

@geido Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments

@github-actions

Copy link
Copy Markdown
Contributor

@geido Ephemeral environment spinning up at http://54.188.1.207:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup.

@michael-s-molina michael-s-molina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Removing the Request Changes status as I'll be out for the next days.

Thanks for the work @amaannawab923! @geido @kasiazjc I'll leave the final review and tests with you. Thanks for the discussions!

@geido
geido merged commit be1b8d6 into apache:master Apr 17, 2025
@amaannawab923 amaannawab923 mentioned this pull request May 1, 2025
9 tasks
alexandrusoare pushed a commit to alexandrusoare/superset that referenced this pull request Jun 19, 2025
Co-authored-by: Amaan Nawab <nelsondrew07@gmail.com>
(cherry picked from commit be1b8d6)
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Co-authored-by: Amaan Nawab <nelsondrew07@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dashboard:native-filters Related to the native filters of the Dashboard size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants