rusackas commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3651903504
########## superset-frontend/playwright/generators/docs/mobile-screenshots.spec.ts: ########## @@ -0,0 +1,167 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Mobile Experience Documentation Screenshot Generator + * + * Captures phone-sized screenshots for the mobile consumption mode docs + * (docs/docs/using-superset/mobile-experience.mdx). Depends on example data + * loaded via `superset load_examples` AND the MOBILE_CONSUMPTION_MODE + * feature flag being enabled in the target environment: + * + * FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True} + * + * Run locally: + * cd superset-frontend + * PLAYWRIGHT_BASE_URL=http://localhost:8088 PLAYWRIGHT_ADMIN_PASSWORD=admin npm run docs:screenshots + * + * Screenshots are saved under docs/static/img/screenshots/mobile/. + */ + +import path from 'path'; +import { Page, test, expect } from '@playwright/test'; +import { URL } from '../../utils/urls'; + +const MOBILE_SCREENSHOTS_DIR = path.resolve( + __dirname, + '../../../../docs/static/img/screenshots/mobile', +); + +// iPhone 12-class viewport; 2x scale factor for crisp docs images +test.use({ + viewport: { width: 390, height: 844 }, + deviceScaleFactor: 2, + hasTouch: true, +}); Review Comment: This one's a manual docs-screenshot generator, not part of CI, meant to be run against whatever local/dev instance the docs contributor already has MOBILE_CONSUMPTION_MODE enabled on (per the file's own header comment). ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,326 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { test, expect, devices } from '@playwright/test'; + +// NOTE: These tests exercise the mobile consumption experience and require +// the MOBILE_CONSUMPTION_MODE feature flag to be enabled in the target +// environment (FEATURE_FLAGS = {"MOBILE_CONSUMPTION_MODE": True}). +import { TIMEOUT } from '../../utils/constants'; +import { URL } from '../../utils/urls'; + +/** + * Mobile dashboard viewing tests verify that dashboards can be viewed + * and interacted with on mobile devices. + * + * These tests assume the World Bank's Health sample dashboard exists. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Dashboard Viewing', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + // Navigate to dashboard list to find a dashboard + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + }); + + test('dashboard list renders in card view on mobile', async ({ page }) => { + // On mobile, dashboard list should show cards, not table + // Look for card elements + const cards = page.locator('[data-test="styled-card"]'); + + // Should have at least one card if dashboards exist + // (This test may need adjustment based on test data availability) + const cardCount = await cards.count(); + + // Either cards are visible, or the empty state is shown; the table + // view must never render on mobile + if (cardCount > 0) { + await expect(cards.first()).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + await expect(page.locator('[data-test="empty-state"]')).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + } + await expect(page.locator('[data-test="listview-table"]')).toHaveCount(0); + }); + + test('mobile search button appears in dashboard list', async ({ page }) => { + // On mobile, the search/filter button should appear in the header + const searchButton = page + .locator('[aria-label="Search"]') + .or(page.locator('[data-test="mobile-search-button"]')); + + // Search button should be visible on mobile + await expect(searchButton.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('tapping dashboard card opens the dashboard', async ({ page }) => { + // Find a dashboard card + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + // Click the first card + await cards.first().click(); + + // Should navigate to dashboard view + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard should load (look for dashboard content) + await expect( + page + .locator('[data-test="dashboard-content-wrapper"]') + .or(page.locator('.dashboard')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + } else { + test.skip(); + } + }); +}); + +test.describe('Mobile Dashboard Interaction', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + // Skip this test suite if no dashboards exist + test.beforeAll(async ({ browser }) => { + const page = await browser.newPage({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + await page.close(); + + if (cardCount === 0) { + test.skip(); + } + }); + + test('dashboard loads and shows charts on mobile', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard to load + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard content should be visible + await expect( + page + .locator('[data-test="dashboard-content-wrapper"]') + .or(page.locator('.dashboard')), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Charts should start loading (look for chart containers) + const chartContainers = page + .locator('[data-test="chart-container"]') + .or(page.locator('.dashboard-chart')); + + // Wait for at least one chart to be visible (with timeout) + await expect(chartContainers.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD * 2, + }); + } + }); + + test('dashboard header shows hamburger menu on mobile', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Look for the hamburger menu / more actions button + const menuButton = page + .locator('[data-test="actions-trigger"]') + .or(page.locator('[aria-label="Menu actions trigger"]')); + + await expect(menuButton.first()).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + } + }); + + test('refresh dashboard works from mobile menu', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Open the actions menu + const menuButton = page + .locator('[data-test="actions-trigger"]') + .or(page.locator('[aria-label="Menu actions trigger"]')); + + if ((await menuButton.count()) > 0) { + await menuButton.first().click(); + + // Look for refresh option + const refreshOption = page.getByText('Refresh dashboard'); + + if ((await refreshOption.count()) > 0) { + await refreshOption.click(); + + // Should show success toast or refresh the charts + // This is hard to verify without checking network requests + // Just verify the menu closes and we're still on the dashboard + await page.waitForTimeout(1000); + expect(page.url()).toMatch(/\/dashboard\/(?!list)/); + } + } + } + }); +}); + +test.describe('Mobile Filter Drawer', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test('filter button appears on dashboards with filters', async ({ page }) => { + // Navigate to dashboard list + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + // Click first dashboard + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + if (cardCount > 0) { + await cards.first().click(); + + // Wait for dashboard + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Give filters time to load + await page.waitForTimeout(2000); + + // Check for filter button (only visible if dashboard has filters) + const filterButton = page + .locator('[data-test="filter-icon"]') + .or( + page + .locator('[aria-label="Filters"]') + .or(page.locator('.mobile-filter-button')), + ); + + const filterCount = await filterButton.count(); + + // The test passes whether filters exist or not + // If filters exist, button should be visible + // If no filters, that's also valid + if (filterCount > 0) { + await expect(filterButton.first()).toBeVisible(); + } Review Comment: Fair, this only asserted anything if the first dashboard in the list happened to have filters. Switched both tests to navigate straight to world_health (already assumed by this file) and skip with a reason if it turns out to have none, instead of quietly no-op'ing. ########## superset-frontend/src/components/ListView/utils.ts: ########## @@ -234,10 +236,19 @@ export function useListViewState({ }; const [viewMode, setViewMode] = useState<ViewModeType>( - (query.viewMode as ViewModeType) || + // forceViewMode overrides everything (used for mobile) + forceViewMode || + (query.viewMode as ViewModeType) || (renderCard ? defaultViewMode : 'table'), ); + // Update viewMode when forceViewMode changes (e.g., screen resize) + useEffect(() => { + if (forceViewMode) { + setViewMode(forceViewMode); + } + }, [forceViewMode]); Review Comment: Good catch, the effect only handled forceViewMode being set, never cleared. Fixed to fall back to the query param or default view when it goes back to undefined. ########## superset-frontend/spec/helpers/mobileTestUtils.ts: ########## @@ -0,0 +1,150 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +/** + * Mobile testing utilities for Jest tests. + * + * Note: We mock 'antd' directly rather than '@superset-ui/core/components' because + * mocking the latter causes circular dependency issues with ActionButton during + * jest.requireActual evaluation. Since Grid is re-exported from antd, mocking + * antd at the source works correctly. + */ + +import { FeatureFlag } from '@superset-ui/core'; + +/** + * Standard mobile breakpoint values (below md breakpoint) + */ +export const mobileBreakpoints = { + xs: true, + sm: true, + md: false, + lg: false, + xl: false, + xxl: false, +}; + +/** + * Standard desktop breakpoint values (at or above md breakpoint) + */ +export const desktopBreakpoints = { + xs: true, + sm: true, + md: true, + lg: true, + xl: true, + xxl: true, +}; + +/** + * Creates a mock for antd Grid.useBreakpoint that returns mobile breakpoints. + * Use this at the top of test files that need to simulate mobile viewport. + * + * @example + * jest.mock('antd', () => mockAntdWithMobileBreakpoint()); + */ +export const mockAntdWithMobileBreakpoint = () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => mobileBreakpoints, + }, +}); + +/** + * Creates a mock for antd Grid.useBreakpoint that returns desktop breakpoints. + * Use this at the top of test files that need to simulate desktop viewport. + * + * @example + * jest.mock('antd', () => mockAntdWithDesktopBreakpoint()); + */ +export const mockAntdWithDesktopBreakpoint = () => ({ + ...jest.requireActual('antd'), + Grid: { + ...jest.requireActual('antd').Grid, + useBreakpoint: () => desktopBreakpoints, + }, +}); + +/** + * Mocks window.matchMedia so `(max-width: ...)` queries match, simulating + * a mobile viewport for the useIsMobile hook. Returns a cleanup function + * restoring the previous matchMedia. Mobile behavior requires BOTH this + * AND the MOBILE_CONSUMPTION_MODE flag (see enableMobileConsumptionFlag). + */ +export const mockMobileMatchMedia = () => { + const previous = window.matchMedia; + Object.defineProperty(window, 'matchMedia', { + writable: true, + value: jest.fn().mockImplementation((query: string) => ({ + matches: query.includes('max-width'), + media: query, + onchange: null, + addListener: jest.fn(), + removeListener: jest.fn(), + addEventListener: jest.fn(), + removeEventListener: jest.fn(), + dispatchEvent: jest.fn(), Review Comment: Fair, switched it to actually parse the max-width value and compare against a simulated viewport width instead of matching any query containing max-width. -- 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]
