sadpandajoe commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3698296127
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -527,7 +546,15 @@ const DashboardBuilder = () => {
const headerContent = useMemo(
() => (
<>
- {!hideDashboardHeader && <DashboardHeader />}
+ {!hideDashboardHeader && (
+ <DashboardHeader
+ onOpenMobileFilters={
+ !isNotMobile && nativeFiltersEnabled && hasFilters
+ ? () => setMobileFiltersOpen(true)
+ : undefined
+ }
Review Comment:
Agreed—customization-only dashboards mount a populated drawer but expose no
trigger because `hasFilters` excludes chart customizations. Should the trigger
use the same filters-or-customizations predicate as `nativeFiltersEnabled`?
##########
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>
+ ) : (
+ <Typography.Link href={dashboardUrl}>
+ {t('Dashboards')}
+ </Typography.Link>
+ ),
+ icon: <Icons.DashboardOutlined />,
+ });
+ }
+
+ // Add theme menu (flatten children directly)
+ menuItems.forEach(item => {
+ if (!item || !('key' in item)) return;
+
+ // Only include theme-sub-menu and language picker
+ if (item.key === 'theme-sub-menu' || item.key === 'language-picker') {
+ items.push({ type: 'divider', key: `divider-before-${item.key}` });
Review Comment:
Agreed—the mobile filter checks `language-picker`, while the language item
is keyed `language-submenu`, so multilingual users never see it. Should this
match the emitted key and add a mobile-menu assertion?
##########
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:
Agreed—unsupported routes mount and can run effects before the passive
effect detects the mobile viewport, contradicting the no-render boundary.
Should the media-query state be initialized synchronously and covered with a
child-mount assertion?
--
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]
