bito-code-review[bot] commented on code in PR #37141:
URL: https://github.com/apache/superset/pull/37141#discussion_r3711324419
##########
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:
<!-- Bito Reply -->
The addition of `onOpenChange={onMenuOpen}` to the mobile `Menu` component
correctly addresses the concern raised by the reviewer. This ensures that the
mobile drawer's submenu interactions now trigger the same upload-permission
checks as the desktop menu, maintaining consistent security logic across
platforms.
##########
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:
<!-- Bito Reply -->
The suggestion to add an explicit return type annotation to the
`useNativeFilters` hook is appropriate. Defining the return shape improves type
safety and helps prevent contract drift as the hook evolves, which is a
standard best practice in TypeScript development.
**superset-frontend/src/dashboard/components/DashboardBuilder/state.ts**
```
export const useNativeFilters = (): {
dashboardFiltersOpen: boolean;
toggleDashboardFiltersOpen: () => void;
nativeFiltersEnabled: boolean;
hasFilters: boolean;
} => {
```
##########
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:
<!-- Bito Reply -->
The suggestion to add tests for the mobile height capping logic is
appropriate. Since the new `heightMultiple` logic introduces critical layout
behavior that is not covered by existing tests, adding these tests ensures the
business logic is verified as required by the project's testing standards.
--
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]
