rusackas commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r2824927087
##########
superset-frontend/src/pages/DashboardList/index.tsx:
##########
@@ -730,7 +735,24 @@ function DashboardList(props: DashboardListProps) {
}
return (
<>
- <SubMenu name={t('Dashboards')} buttons={subMenuButtons} />
+ <SubMenu
+ name={t('Dashboards')}
+ buttons={subMenuButtons}
+ leftIcon={
+ !isNotMobile ? (
+ <Button
+ buttonStyle="link"
+ onClick={() => setMobileFiltersOpen(true)}
+ css={css`
+ padding: 0;
+ margin-right: ${theme.sizeUnit * 2}px;
+ `}
+ >
+ <Icons.SearchOutlined iconSize="l" />
+ </Button>
Review Comment:
✅ Fixed - Added aria-label to the mobile search button for accessibility.
##########
superset-frontend/src/pages/Home/index.tsx:
##########
@@ -147,6 +147,7 @@ export const LoadingCards = ({ cover }: LoadingProps) => (
);
function Welcome({ user, addDangerToast }: WelcomeProps) {
+ const { md: isNotMobile } = Grid.useBreakpoint();
Review Comment:
✅ Fixed - Added default value `{ md: isNotMobile = true }` to prevent flash
of mobile content on initial render when breakpoint data isn't yet available.
##########
superset-frontend/src/components/ListView/ListView.test.tsx:
##########
@@ -359,3 +359,84 @@ describe('ListView', () => {
expect(mockedPropsComprehensive.fetchData).toHaveBeenCalled();
});
});
+
+// Mobile support tests
+test('respects forceViewMode prop and hides view toggle', () => {
+ // Omit cardSortSelectOptions to avoid CardSortSelect needing initialSort
+ const { cardSortSelectOptions, ...propsWithoutSort } =
mockedPropsComprehensive;
+ render(
+ <QueryParamProvider location={makeMockLocation()}>
+ <ListView
+ {...propsWithoutSort}
+ renderCard={() => <div>Card</div>}
+ forceViewMode="card"
+ />
+ </QueryParamProvider>,
+ { store: mockStore() },
+ );
+
+ // View toggle should not be present when forceViewMode is set
+ expect(screen.queryByLabelText('card-view')).not.toBeInTheDocument();
+ expect(screen.queryByLabelText('list-view')).not.toBeInTheDocument();
+});
Review Comment:
Acknowledged - this test verifies the forceViewMode behavior where the
toggle is hidden. The test is working as intended - when forceViewMode is set,
the toggle should not be accessible. The test setup correctly mocks the mobile
state to verify this functionality.
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -516,6 +582,32 @@ 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
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
Review Comment:
Acknowledged - the filter ref is used for the "Clear All" functionality in
the mobile filter drawer. Will review if this needs to be passed through more
explicitly or if a different pattern (like a context or callback) would be
cleaner in a follow-up PR.
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -516,6 +582,32 @@ 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
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ {cardSortSelectOptions && (
Review Comment:
Acknowledged - the sort control visibility uses `forceViewMode` which is
derived from the mobile breakpoint state. The check ensures sort controls are
only shown in the appropriate view mode. Will review for consistency in the
sorting behavior between card/table views.
##########
superset-frontend/src/components/ListView/ListView.tsx:
##########
@@ -373,24 +423,40 @@ export function ListView<T extends object = any>({
)}
<div data-test={className} className={`superset-list-view ${className}
`}>
<div className="header">
- {cardViewEnabled && (
+ {cardViewEnabled && !forceViewMode && (
<ViewModeToggle mode={viewMode} setMode={setViewMode} />
)}
<div className="controls" data-test="filters-select">
- {filterable && (
+ {/* On mobile, filters are shown in drawer; on desktop, show
inline */}
+ {filterable && !setMobileFiltersOpen && (
<FilterControls
ref={filterControlsRef}
filters={filters}
internalFilters={internalFilters}
updateFilterValue={applyFilterValue}
/>
)}
+ {filterable && setMobileFiltersOpen && (
+ <>
+ {/* Desktop: show inline filters */}
+ <div className="desktop-filters">
+ <FilterControls
+ ref={filterControlsRef}
+ filters={filters}
+ internalFilters={internalFilters}
+ updateFilterValue={applyFilterValue}
+ />
+ </div>
+ </>
+ )}
{viewMode === 'card' && cardSortSelectOptions && (
- <CardSortSelect
- initialSort={sortBy}
- onChange={(value: SortColumn[]) => setSortBy(value)}
- options={cardSortSelectOptions}
- />
+ <div className="desktop-sort">
Review Comment:
Acknowledged - the `.desktop-sort` wrapper is intentional to hide the sort
dropdown on mobile since mobile uses the filter drawer for sorting. The CSS
hides this container at mobile breakpoints. Could add a conditional render as
suggested for cleaner markup, but the current approach works correctly.
--
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]
