rusackas commented on code in PR #37141: URL: https://github.com/apache/superset/pull/37141#discussion_r3707303327
########## superset-frontend/src/hooks/useIsMobile.ts: ########## @@ -0,0 +1,71 @@ +/** + * 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 { useEffect, useState } from 'react'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; +import { useTheme } from '@apache-superset/core/theme'; + +// Matches antd's screenSMMax token; used only when no theme is in scope. +const FALLBACK_MOBILE_MAX_WIDTH = 767; + +/** + * Whether the mobile consumption-only experience is enabled for this + * deployment. Non-hook variant for use inside styled-component + * interpolations; prefer `useIsMobile` in components. + */ +export function isMobileConsumptionEnabled(): boolean { + return isFeatureEnabled(FeatureFlag.MobileConsumptionMode); +} + +/** + * Returns true when MOBILE_CONSUMPTION_MODE is enabled AND the viewport is + * at or below the theme's `screenSMMax` breakpoint. All mobile-specific + * behavior (route guarding, consumption-only chrome, drawer navigation) + * should key off this hook so the flag remains a single kill switch. + * + * The matchMedia subscription is only installed when the flag is on, and + * state only changes when the match flips, so with the flag off (or on + * desktop) this hook never causes a re-render — consumers are inert. + * + * The initial value is always false (desktop), so the first paint never + * takes the mobile branch by accident. + */ +export function useIsMobile(): boolean { + const enabled = isMobileConsumptionEnabled(); + const theme = useTheme(); + const maxWidth = theme?.screenSMMax ?? FALLBACK_MOBILE_MAX_WIDTH; + const [isSmallScreen, setIsSmallScreen] = useState(false); + + useEffect(() => { + if (!enabled) { + return undefined; + } + const mediaQuery = window.matchMedia(`(max-width: ${maxWidth}px)`); + const update = () => setIsSmallScreen(mediaQuery.matches); Review Comment: Right, the effect only runs after commit, so on a small screen the guarded route would mount for one commit before the mobile check flips it. Pushed 0dfb778b: seeded the state with a lazy initializer that reads matchMedia synchronously on first render (falling back to desktop when window/matchMedia is unavailable), and kept the effect for subscription updates. ########## superset-frontend/src/features/home/RightMenu.tsx: ########## @@ -644,6 +654,64 @@ const RightMenu = ({ handleLogout, ]); + // Build mobile menu items - consumption only (no create/admin actions) + const mobileMenuItems = useMemo(() => { + const items: MenuItem[] = []; + + // Add Dashboards link at top (from main menu) + // Match on the FAB-internal `name`, which is stable across locales + // (`label` is translated and would break in non-English deployments) + const dashboardsMenu = menu?.find(item => item.name === 'Dashboards'); + if (dashboardsMenu) { + const dashboardUrl = dashboardsMenu.url || '/dashboard/list/'; + items.push({ + key: 'dashboards', + label: isFrontendRoute(dashboardUrl) ? ( + <Link to={dashboardUrl}>{t('Dashboards')}</Link> Review Comment: Good catch, confirmed - this one was missed when the rest of the menu items in this file already go through stripAppRoot/ensureAppRoot. Pushed 0dfb778b applying the same treatment to the mobile Dashboards link. ########## superset-frontend/playwright/tests/mobile/mobile-dashboard.spec.ts: ########## @@ -0,0 +1,288 @@ +/** + * 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, Page } 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']; + +/** + * Navigates to the dashboard list, clicks the first available dashboard + * card, and waits for navigation into that dashboard. Skips the current + * test when no dashboards are available to open. + */ +async function openFirstDashboard(page: Page): Promise<void> { + await page.goto(URL.DASHBOARD_LIST); + await page.waitForLoadState('networkidle'); + + const cards = page.locator('[data-test="styled-card"]'); + const cardCount = await cards.count(); + + test.skip(cardCount === 0, 'No dashboards available to open on mobile'); + + await cards.first().click(); + + await page.waitForURL(url => /\/dashboard\/(?!list)/.test(url.pathname), { + timeout: TIMEOUT.PAGE_LOAD, + }); +} + +/** + * Navigates to the World Bank's Health dashboard and returns a locator + * for its mobile filter button. Skips the current test when the fixture + * has no native filters configured. + */ +async function getMobileFilterButton(page: Page) { + // Navigate directly to the World Bank's Health dashboard, which this + // spec's fixtures require, rather than an arbitrary first card from + // the list. Whether it has native filters configured depends on the + // fixture, so callers skip themselves when none are present. + await page.goto('dashboard/world_health/'); + await page.waitForLoadState('networkidle'); + + // Give filters time to load + await page.waitForTimeout(2000); + + const filterButton = page + .locator('[data-test="filter-icon"]') Review Comment: Confirmed - the trigger renders with data-test="mobile-filters-trigger" and aria-label="Open filters", neither of which the old locators matched, so these tests always hit the skip branch. Pushed 0dfb778b fixing the locator to match the real trigger. ########## superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx: ########## @@ -205,6 +218,52 @@ export const useHeaderActionsMenu = ({ const menuItems: MenuItem[] = []; + // Mobile-only: show dashboard info items in menu + if (isMobile && !editMode) { Review Comment: Good catch. Save as and Embed dashboard are both authoring actions that should not be in a consumption-only menu. Pushed 0dfb778b gating both behind !isMobile, same as the fullscreen toggle and report dropdown already do in this file. -- 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]
