rusackas commented on code in PR #38193:
URL: https://github.com/apache/superset/pull/38193#discussion_r3360302046
##########
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:
Good catch — you're right that `items.length > 0` was way broader than the
flicker window and left a permanent 0-badge button that swallowed clicks on
wide viewports. Fixed in 5d6d681391 by taking your suggested approach: removed
the `alwaysShowDropdownButton` prop entirely and moved the logic inside
`DropdownContainer`. It now tracks a `recalculating` flag (set when an item-set
change resets the overflow index, cleared once the real index is recomputed)
and keeps the trigger mounted across that transient only when it was showing
content just before (`usePrevious(!!popoverContent)`). In steady state the
trigger appears exactly when there's overflow content, matching pre-fix
behavior — no more lingering empty button.
##########
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:
Agreed — that test didn't isolate the `onOpenChange` guard since
`open={popoverVisible && !!popoverContent}` was a sufficient independent guard.
Since the new approach (5d6d681391) drops the `alwaysShowDropdownButton` prop,
I removed that test along with the other two prop-specific ones rather than
leave misleading coverage. The steady-state guarantee you cared about (no
button when nothing overflows) is already covered by the existing `does not
render a dropdown button when not overflowing` test.
--
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]
