rusackas commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3658992218
########## 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', +); Review Comment: Good catch, fixed! The directory's committed to the repo alongside the images so this never actually hit, but mkdir'ing it defensively costs nothing and covers a standalone re-run against a pruned checkout. ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,327 @@ +/** + * 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, + }); + Review Comment: This one's gated a level up rather than in the spec... mobile specs are excluded from the default chromium project's testMatch and only picked up by `chromium-mobile`, which only exists when `INCLUDE_MOBILE=true` (mirrors how the embedded suite is gated). The CI workflow sets that plus `SUPERSET_FEATURE_MOBILE_CONSUMPTION_MODE=true` for the mobile step, so there's no path where this file runs against a flag-off backend. ########## superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts: ########## @@ -0,0 +1,192 @@ +/** + * 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 { URL } from '../../utils/urls'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile navigation tests verify the MobileRouteGuard behavior + * and mobile-specific navigation patterns. + * + * These tests run with a mobile viewport to trigger mobile-specific behavior. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Navigation', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + await page.goto('/'); + }); Review Comment: Same as the note on `mobile-dashboard.spec.ts`... gating happens in `playwright.config.ts` (the `chromium-mobile` project, only registered when `INCLUDE_MOBILE=true`) rather than in the spec itself. ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx: ########## @@ -101,6 +102,19 @@ const StyledHeader = styled.div<{ filterBarWidth: number }>` z-index: 99; max-width: calc(100vw - ${filterBarWidth}px); + /* Mobile consumption mode: let the dashboard title scroll away and keep + only the tab bar sticky. A pinned title would sit underneath the + higher-z sticky tabs, leaving its bottom edge (kebab button) peeking + out below the tab bar. */ + ${ + isMobileConsumptionEnabled() && + css` + @media (max-width: ${theme.screenSMMax}px) { + position: relative; + } + ` + } Review Comment: I think this one's off... screenSMMax is 767 in antd's default theme (screenMDMin is 768), so `max-width: ${theme.screenSMMax}px\}` is the same threshold as `useIsMobile()`'s md check, not the ~575px sm breakpoint. That's screenXSMax. Happy to swap in a named constant if the theme-token indirection is the part that's hard to follow though. ########## superset-frontend/src/dashboard/components/DashboardBuilder/DashboardWrapper.tsx: ########## @@ -110,6 +111,22 @@ const StyledDiv = styled.div` i.warning { color: ${theme.colorWarning}; } + + /* Mobile consumption mode: show the full chart title without + truncation (controls and links are render-gated in SliceHeader) */ + ${ + isMobileConsumptionEnabled() + ? `@media (max-width: ${theme.screenSMMax}px) { + [data-test='slice-header'] .header-title { + -webkit-line-clamp: unset; + display: block; + white-space: normal; + overflow: visible; + text-overflow: unset; + } + }` + : '' + } Review Comment: I think this one's off... screenSMMax is 767 in antd's default theme (screenMDMin is 768), so `max-width: ${theme.screenSMMax}px\}` is the same threshold as `useIsMobile()`'s md check, not the ~575px sm breakpoint. That's screenXSMax. Happy to swap in a named constant if the theme-token indirection is the part that's hard to follow though. ########## superset-frontend/packages/superset-ui-core/src/components/PageHeaderWithActions/index.tsx: ########## @@ -82,6 +83,20 @@ const headerStyles = (theme: SupersetTheme) => css` display: flex; align-items: center; } + + /* Mobile consumption mode: center the title between left/right panels */ + ${ + isFeatureEnabled(FeatureFlag.MobileConsumptionMode) && + css` + @media (max-width: ${theme.screenSMMax}px) { + .title-panel { + flex: 1; + justify-content: center; + margin-right: 0; + } + } + ` Review Comment: I think this one's off... screenSMMax is 767 in antd's default theme (screenMDMin is 768), so `max-width: ${theme.screenSMMax}px\}` is the same threshold as `useIsMobile()`'s md check, not the ~575px sm breakpoint. That's screenXSMax. Happy to swap in a named constant if the theme-token indirection is the part that's hard to follow though. ########## superset-frontend/src/components/ListView/CardCollection.tsx: ########## @@ -42,6 +43,18 @@ const CardContainer = styled.div<{ showThumbnails?: boolean }>` ? `${theme.sizeUnit * 8 + 3}px ${theme.sizeUnit * 20}px` : `${theme.sizeUnit * 8 + 1}px ${theme.sizeUnit * 20}px` }; + + /* Full-width cards on mobile (consumption mode) */ + ${ + isMobileConsumptionEnabled() + ? `@media (max-width: ${theme.screenSMMax}px) { + grid-template-columns: 1fr; + grid-gap: ${theme.sizeUnit * 4}px; + padding-left: ${theme.sizeUnit * 4}px; + padding-right: ${theme.sizeUnit * 4}px; + }` + : '' Review Comment: I think this one's off... screenSMMax is 767 in antd's default theme (screenMDMin is 768), so `max-width: ${theme.screenSMMax}px\}` is the same threshold as `useIsMobile()`'s md check, not the ~575px sm breakpoint. That's screenXSMax. Happy to swap in a named constant if the theme-token indirection is the part that's hard to follow though. ########## superset-frontend/playwright/tests/mobile/mobile-navigation.spec.ts: ########## @@ -0,0 +1,192 @@ +/** + * 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 { URL } from '../../utils/urls'; +import { TIMEOUT } from '../../utils/constants'; + +/** + * Mobile navigation tests verify the MobileRouteGuard behavior + * and mobile-specific navigation patterns. + * + * These tests run with a mobile viewport to trigger mobile-specific behavior. + */ + +// Use iPhone 12 viewport for mobile tests +const mobileViewport = devices['iPhone 12']; + +test.describe('Mobile Navigation', () => { + test.use({ + viewport: mobileViewport.viewport, + userAgent: mobileViewport.userAgent, + }); + + test.beforeEach(async ({ page }) => { + await page.goto('/'); + }); + + test('mobile viewport redirects from chart list to MobileUnsupported page', async ({ + page, + }) => { + // Navigate to chart list (not mobile-supported) + await page.goto(URL.CHART_LIST); + + // Should show the MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Primary action buttons should be visible + await expect( + page.getByRole('button', { name: 'View Dashboards' }), + ).toBeVisible(); + await expect( + page.getByRole('button', { name: 'Go to Welcome Page' }), + ).toBeVisible(); + }); + + test('mobile viewport allows access to dashboard list', async ({ page }) => { + // Navigate to dashboard list (mobile-supported) + await page.goto(URL.DASHBOARD_LIST); + + // Should NOT show MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).not.toBeVisible({ timeout: TIMEOUT.FORM_LOAD }); + + // Should show dashboard list content (look for dashboard list elements) + await expect( + page + .locator('[data-test="listview-table"]') + .or(page.locator('[data-test="styled-card"]')) + .first(), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + }); + + test('mobile viewport allows access to welcome page', async ({ page }) => { + // Navigate to welcome page (mobile-supported) + await page.goto(URL.WELCOME); + + // Should NOT show MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).not.toBeVisible({ timeout: TIMEOUT.FORM_LOAD }); + + // Should show welcome page content + await expect( + page.getByText('Recents').or(page.getByText('Dashboards')).first(), + ).toBeVisible({ + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('View Dashboards button navigates to dashboard list', async ({ + page, + }) => { + // Navigate to unsupported route + await page.goto(URL.CHART_LIST); + + // Wait for MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Click View Dashboards button + await page.getByRole('button', { name: 'View Dashboards' }).click(); + + // Should navigate to dashboard list + await page.waitForURL(url => url.pathname.includes('dashboard/list'), { + timeout: TIMEOUT.PAGE_LOAD, + }); + + // Dashboard list should be accessible + await expect( + page + .locator('[data-test="listview-table"]') + .or(page.locator('[data-test="styled-card"]')) + .first(), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + }); + + test('Go to Welcome Page button navigates to welcome', async ({ page }) => { + // Navigate to unsupported route + await page.goto(URL.CHART_LIST); + + // Wait for MobileUnsupported page + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + // Click Go to Welcome Page button + await page.getByRole('button', { name: 'Go to Welcome Page' }).click(); + + // Should navigate to welcome page + await page.waitForURL(url => url.pathname.includes('welcome'), { + timeout: TIMEOUT.PAGE_LOAD, + }); + }); + + test('unsupported screen offers no bypass', async ({ page }) => { + // The "Continue anyway" bypass was removed: desktop views are unusable + // at phone width, and growing the viewport unblocks routes automatically + await page.goto(URL.CHART_LIST); + + await expect( + page.getByText("This view isn't available on mobile"), + ).toBeVisible({ timeout: TIMEOUT.PAGE_LOAD }); + + await expect(page.getByText('Continue anyway')).toHaveCount(0); + }); Review Comment: Good catch on the inconsistency, though it's the PR description that's stale, not the test... the bypass was removed a few commits back (growing the viewport past the breakpoint unblocks the route automatically now), and the test is correctly asserting it's gone. Updated the description to match. -- 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]
