rusackas commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3711322126
##########
superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:
##########
@@ -182,11 +197,29 @@ const ChartHolder = ({
}, [
component,
getComponentById,
+ isMobile,
+ editMode,
parentComponent.meta.width,
parentComponent.parents,
parentComponent.type,
]);
+ // Grid units of height for this chart. In mobile consumption mode the
+ // authored desktop height is capped to the viewport (minus app chrome) so
+ // tall charts don't dominate the single-column stacked layout. Used for
+ // both the ResizableContainer shell and the height handed to the plugin,
+ // so the two can't disagree.
+ const heightMultiple = useMemo(() => {
+ const authoredHeight = component.meta.height ?? GRID_MIN_ROW_UNITS;
+ if (isMobile && !editMode) {
+ const maxUnits = Math.floor(
+ (window.innerHeight - MOBILE_CHROME_HEIGHT) / GRID_BASE_UNIT,
+ );
+ return Math.max(GRID_MIN_ROW_UNITS, Math.min(authoredHeight, maxUnits));
+ }
+ return authoredHeight;
+ }, [component.meta.height, isMobile, editMode]);
Review Comment:
Fixed - tracked viewport height via a resize/orientationchange listener
while mobile so heightMultiple stays reactive instead of freezing at whatever
height was current when the mobile check last flipped. Pushed 14381d3f.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -372,6 +386,8 @@ const DashboardBuilder = () => {
const dispatch = useDispatch();
const uiConfig = useUiConfig();
const theme = useTheme();
+ const isNotMobile = !useIsMobile();
+ const [mobileFiltersOpen, setMobileFiltersOpen] = useState(false);
Review Comment:
Fixed - mobileFiltersOpen now resets when leaving mobile mode. Pushed
14381d3f.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/state.ts:
##########
@@ -121,5 +121,6 @@ export const useNativeFilters = () => {
dashboardFiltersOpen,
toggleDashboardFiltersOpen,
nativeFiltersEnabled,
+ hasFilters: filterValues.length > 0 || chartCustomizations.length > 0,
Review Comment:
Added a UseNativeFiltersResult return type. Pushed 14381d3f.
##########
superset-frontend/src/features/home/RightMenu.tsx:
##########
@@ -702,48 +770,98 @@ const RightMenu = ({
</Tag>
);
})()}
- <Menu
- css={css`
- display: flex;
- flex-direction: row;
- align-items: center;
- height: 100%;
- border-bottom: none !important;
-
- /* Remove the underline from menu items */
- .ant-menu-item:after,
- .ant-menu-submenu:after {
- content: none !important;
- }
-
- .submenu-with-caret {
+ {/* Mobile: hamburger menu with drawer */}
+ {isMobile && (
+ <>
+ <Button
+ buttonStyle="link"
+ onClick={() => setMobileMenuOpen(true)}
+ aria-label={t('Menu')}
+ >
+ <Icons.MenuOutlined iconSize="l" />
+ </Button>
+ <Drawer
+ title={null}
+ placement="right"
+ onClose={() => setMobileMenuOpen(false)}
+ open={mobileMenuOpen}
+ width={280}
+ styles={{
+ header: { display: 'none' },
+ body: { padding: 0 },
+ }}
+ >
+ <Menu
+ mode="inline"
+ selectable={false}
+ onClick={info => {
+ handleMenuSelection(info);
+ // The reused desktop items navigate via anchors that only
+ // span their label text, but the drawer's tap target is the
+ // full menu row — navigate explicitly so row taps work.
+ if (info.key === 'info' && navbarRight.user_info_url) {
+ navigateTo(navbarRight.user_info_url);
+ return;
+ }
+ if (info.key === 'logout' && navbarRight.user_logout_url) {
+ navigateTo(navbarRight.user_logout_url);
+ return;
+ }
+ setMobileMenuOpen(false);
+ }}
+ items={mobileMenuItems}
+ css={css`
+ border-inline-end: none !important;
+ `}
+ />
Review Comment:
Good catch, added onOpenChange={onMenuOpen} to the mobile Menu so Data
submenu opens trigger the same upload-permission checks the desktop menu
already runs. Pushed 14381d3f.
##########
superset-frontend/src/dashboard/components/gridComponents/ChartHolder/ChartHolder.tsx:
##########
@@ -182,11 +197,29 @@ const ChartHolder = ({
}, [
component,
getComponentById,
+ isMobile,
+ editMode,
parentComponent.meta.width,
parentComponent.parents,
parentComponent.type,
]);
+ // Grid units of height for this chart. In mobile consumption mode the
+ // authored desktop height is capped to the viewport (minus app chrome) so
+ // tall charts don't dominate the single-column stacked layout. Used for
+ // both the ResizableContainer shell and the height handed to the plugin,
+ // so the two can't disagree.
+ const heightMultiple = useMemo(() => {
+ const authoredHeight = component.meta.height ?? GRID_MIN_ROW_UNITS;
+ if (isMobile && !editMode) {
+ const maxUnits = Math.floor(
+ (window.innerHeight - MOBILE_CHROME_HEIGHT) / GRID_BASE_UNIT,
+ );
+ return Math.max(GRID_MIN_ROW_UNITS, Math.min(authoredHeight, maxUnits));
+ }
+ return authoredHeight;
+ }, [component.meta.height, isMobile, editMode]);
Review Comment:
Added tests in ChartHolder.test.tsx covering the mobile height cap: capping
a tall authored height to the viewport, flooring at GRID_MIN_ROW_UNITS on very
short viewports, and leaving the authored height untouched in edit mode. Pushed
14381d3f.
--
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]
