aminghadersohi commented on code in PR #38193:
URL: https://github.com/apache/superset/pull/38193#discussion_r3359593438
##########
superset-frontend/packages/superset-ui-core/src/components/DropdownContainer/DropdownContainer.test.tsx:
##########
@@ -158,6 +158,36 @@ test('accepts custom style props', () => {
expect(container).toHaveStyle('padding: 10px');
});
+test('shows dropdown button when alwaysShowDropdownButton is true even without
overflow', () => {
+ render(
+ <DropdownContainer items={generateItems(3)} alwaysShowDropdownButton />,
+ );
+ expect(screen.getByTestId('dropdown-container-btn')).toBeInTheDocument();
+});
+
+test('does not open popover when alwaysShowDropdownButton is true but there is
no popover content', () => {
+ render(
+ <DropdownContainer items={generateItems(3)} alwaysShowDropdownButton />,
+ );
+ const btn = screen.getByTestId('dropdown-container-btn');
+ expect(btn).toBeInTheDocument();
+ fireEvent.click(btn);
+ // No popover content exists, so the popover should not open
+ expect(screen.queryByRole('tooltip')).not.toBeInTheDocument();
Review Comment:
**NIT** — this assertion would still pass if the `if (popoverContent)` guard
inside `onOpenChange` were removed entirely, because `open={popoverVisible &&
!!popoverContent}` is a sufficient independent guard that prevents the Popover
from rendering regardless. The test validates the desired end-state correctly
but does not isolate the `onOpenChange` mechanism it claims to cover.
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx:
##########
@@ -641,6 +641,7 @@ const FilterControls: FC<FilterControlsProps> = ({
: undefined
}
forceRender={hasRequiredFirst}
+ alwaysShowDropdownButton={items.length > 0}
Review Comment:
**MEDIUM (non-blocking)** — `alwaysShowDropdownButton={items.length > 0}` is
broader than the recalculation window it targets. On wide viewports with few
filters / no overflow / no out-of-scope content, `popoverContent` stays `null`
indefinitely, so the "More filters" button renders permanently with a 0-badge
and silently swallows clicks (the `onOpenChange` guard blocks all interaction).
This is a behavior change from pre-fix: the button previously only appeared
with overflow content to show.
The JSDoc documents the trade-off, so flagging rather than blocking. A
narrower approach — e.g. a one-cycle ref flag set when a resize fires and
cleared after the first `useLayoutEffect` recalculation pass — would achieve
the same flicker prevention with no steady-state UX regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
