rusackas commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r2824927635
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -506,7 +516,15 @@ const DashboardBuilder = () => {
const renderDraggableContent = useCallback(
({ dropIndicatorProps }: { dropIndicatorProps: JsonObject }) => (
<div>
- {!hideDashboardHeader && <DashboardHeader />}
+ {!hideDashboardHeader && (
+ <DashboardHeader
+ onOpenMobileFilters={
+ !isNotMobile && nativeFiltersEnabled && hasFilters
Review Comment:
Acknowledged - the filter drawer button only appears when `hasFilters` is
true, which is correct behavior. If there are no native filters, there's
nothing to show in the drawer. Will verify that the component handles edge
cases (like loading states) appropriately.
##########
superset-frontend/src/dashboard/components/Header/useHeaderActionsDropdownMenu.tsx:
##########
@@ -187,6 +201,46 @@ export const useHeaderActionsMenu = ({
const menuItems: MenuItem[] = [];
+ // Mobile-only: show dashboard info items in menu
+ if (isMobile && !editMode) {
+ // Favorite toggle
+ if (saveFaveStar) {
+ menuItems.push({
+ key: 'toggle-favorite',
+ label: isStarred ? t('Remove from favorites') : t('Add to
favorites'),
+ });
+ }
+
+ // Published status
+ menuItems.push({
+ key: 'status-info',
+ label: isPublished ? t('Status: Published') : t('Status: Draft'),
+ disabled: true,
+ });
+
+ // Owner info
+ const ownerNames =
+ dashboardInfo?.owners?.length > 0
+ ? dashboardInfo.owners.map(getOwnerName).join(', ')
+ : t('None');
+ menuItems.push({
+ key: 'owner-info',
+ label: `${t('Owner')}: ${ownerNames}`,
+ disabled: true,
+ });
+
+ // Last modified
+ const modifiedBy =
+ getOwnerName(dashboardInfo?.changed_by) || t('Not available');
+ menuItems.push({
+ key: 'modified-info',
+ label: `${t('Modified')} ${dashboardInfo?.changed_on_delta_humanized
|| ''} ${t('by')} ${modifiedBy}`,
Review Comment:
Acknowledged - the i18n string concatenation issue is a good catch. Using
interpolation (`t('Modified {date} by {user}', {date: ..., user: ...})`) would
be more translation-friendly. Will address in a follow-up to improve i18n
support.
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -549,6 +615,33 @@ export function ListView<T extends object = any>({
)}
</div>
</div>
+
+ {/* Mobile filter drawer */}
+ {filterable && setMobileFiltersOpen && (
+ <Drawer
+ title={mobileFiltersDrawerTitle || t('Search')}
+ placement="left"
+ onClose={() => setMobileFiltersOpen(false)}
+ open={mobileFiltersOpen}
+ width={300}
+ >
+ <MobileFilterDrawerContent>
+ <FilterControls
+ ref={filterControlsRef}
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ {viewMode === 'card' && cardSortSelectOptions && (
+ <CardSortSelect
+ initialSort={sortBy}
+ onChange={(value: SortColumn[]) => setSortBy(value)}
+ options={cardSortSelectOptions}
+ />
+ )}
+ </MobileFilterDrawerContent>
Review Comment:
Acknowledged - the FilterControls rendering is intentional: when
`setMobileFiltersOpen` is provided, we render FilterControls in the header for
desktop view AND inside the drawer for mobile view. The duplicate is controlled
by CSS breakpoints so only one is visible at a time. Could refactor to
conditional rendering for cleaner code.
##########
superset-frontend/src/components/ListView/utils.ts:
##########
@@ -243,10 +245,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:
Acknowledged - the viewMode state reactivity concern is valid. The current
implementation initializes viewMode once from localStorage/breakpoint. If the
screen resizes after initial load, it doesn't automatically switch. This is
intentional to preserve user preference, but could add a resize listener if
auto-switching is desired.
##########
superset-frontend/src/features/home/Menu.test.tsx:
##########
@@ -24,6 +24,15 @@ import { getExtensionsRegistry } from '@superset-ui/core';
import { Menu } from './Menu';
import * as getBootstrapData from 'src/utils/getBootstrapData';
+// Mock useBreakpoint to return desktop breakpoints (prevents mobile menu
rendering)
+jest.mock('antd', () => ({
+ ...jest.requireActual('antd'),
+ Grid: {
+ ...jest.requireActual('antd').Grid,
+ useBreakpoint: () => ({ xs: true, sm: true, md: true, lg: true, xl: true
}),
+ },
+}));
Review Comment:
Acknowledged - the test mock uses jest.mock for the antd Grid.useBreakpoint
hook. This was added to fix test failures where components couldn't find
elements because the mock returned mobile dimensions. The approach of mocking
antd directly works because @superset-ui/core/components re-exports from antd.
--
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]
