bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3717521437
##########
docs/docs/using-superset/mobile-experience.mdx:
##########
@@ -0,0 +1,89 @@
+---
+title: Mobile Experience
+sidebar_position: 7
+version: 1
+---
+
+import useBaseUrl from "@docusaurus/useBaseUrl";
+
+# Mobile Experience
+
+Superset ships an optional, consumption-only mobile experience for viewing
+dashboards on phones and other small screens. When enabled, screens below
+768px wide get a layout built for touch: dashboards render their charts
Review Comment:
<!-- Bito Reply -->
The suggestion to update the documentation breakpoint to '767px' is valid.
While the documentation previously stated 'below 768px', the actual
implementation uses a 767px threshold (as seen in the code's `screenSMMax` and
`useIsMobile` fallback). Aligning the documentation with the code's specific
breakpoint improves clarity and consistency.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.test.tsx:
##########
@@ -963,3 +979,62 @@ test('withholds the empty-state edit action while
previewing a version', async (
queryByRole('button', { name: 'Edit the dashboard' }),
).not.toBeInTheDocument();
});
+
+// Mobile support tests
+// Note: The main mobile tests require mocking useBreakpoint to return mobile
breakpoints
+// which is done at the module level. These tests verify mobile-related
component behavior.
+
+test('should not render filter bar panel on desktop when nativeFiltersEnabled
is false', () => {
+ (useStoredSidebarWidth as jest.Mock).mockImplementation(() => [
+ 100,
+ jest.fn(),
+ ]);
+ (fetchFaveStar as jest.Mock).mockReturnValue({ type: 'mock-action' });
+ (setActiveTab as jest.Mock).mockReturnValue({ type: 'mock-action' });
+
+ jest.spyOn(useNativeFiltersModule, 'useNativeFilters').mockReturnValue({
+ showDashboard: true,
+ missingInitialFilters: [],
+ dashboardFiltersOpen: true,
+ toggleDashboardFiltersOpen: jest.fn(),
+ nativeFiltersEnabled: false,
+ hasFilters: false,
+ });
+
+ const { queryByTestId } = render(<DashboardBuilder />, {
+ useRedux: true,
+ store: storeWithState({
+ ...mockState,
+ dashboardLayout: undoableDashboardLayout,
+ }),
+ useDnd: true,
+ useTheme: true,
+ useRouter: true,
+ });
+
+ // Filter panel should not be present when native filters are disabled
+ expect(queryByTestId('dashboard-filters-panel')).not.toBeInTheDocument();
+});
Review Comment:
<!-- Bito Reply -->
The decision to remove the test is appropriate. Since the test was
identified as a duplicate of an existing assertion, removing it helps maintain
the test suite and prevents maintenance divergence.
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -229,7 +230,9 @@ const SliceHeader = forwardRef<HTMLDivElement,
SliceHeaderProps>(
0,
);
- const canExplore = !editMode && supersetCanExplore;
+ // Consumption-only mobile mode: no explore link, no chart controls
+ const isMobile = useIsMobile();
Review Comment:
<!-- Bito Reply -->
The suggestion to add a mobile-viewport test is appropriate. Since the new
logic introduces conditional behavior based on the mobile state, verifying this
with a test ensures that the `isMobile` gating correctly hides the explore link
and chart controls as intended, preventing future regressions.
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -51,9 +51,16 @@ import {
Icons,
Typography,
TelemetryPixel,
+ Drawer,
+ Button,
} from '@superset-ui/core/components';
import type { ItemType, MenuItem } from '@superset-ui/core/components/Menu';
-import { ensureAppRoot, stripAppRoot } from 'src/utils/navigationUtils';
+import {
+ ensureAppRoot,
+ navigateTo,
+ stripAppRoot,
+} from 'src/utils/navigationUtils';
+import { useIsMobile } from 'src/hooks/useIsMobile';
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion is valid. The new mobile tests in
`superset-frontend/src/features/home/SubMenu.test.tsx` use `data-test`
attributes, but the tests query elements using `findByTestId`, which
specifically looks for `data-testid` attributes. This mismatch causes the tests
to fail to locate the elements.
To resolve this, update the `data-test` attributes to `data-testid` in the
test setup, as shown below:
**superset-frontend/src/features/home/SubMenu.test.tsx**
```
+test('should render leftIcon when provided', async () => {
+ setup({
+ leftIcon: (
+ <button type="button" data-testid="left-icon-button">
+ Search
+ </button>
+ ),
+ });
+ expect(await screen.findByTestId('left-icon-button')).toBeInTheDocument();
+});
```
--
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]
