sfirke commented on code in PR #43940:
URL: https://github.com/apache/superset/pull/43940#discussion_r3969164021
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -774,6 +776,17 @@ const DashboardBuilder = () => {
? theme.sizeUnit * 4
: theme.sizeUnit * 8;
+ // Tab bars nested in the grid pin just below the sticky header while the
+ // page scrolls. Not in the mobile viewport, where the header scrolls away
+ // and the mobile styling pins tab bars on its own; not in report mode,
+ // whose tiled screenshots scroll the page and would capture a pinned bar
+ // in every tile; and not while a chart is maximized, which sits inside its
+ // own stacking context and must not be covered by a pinned bar.
+ // (TabsRenderer itself opts out while editing, since drop targets rely on
+ // document flow.)
+ const stickyTabsOffset =
+ isMobileViewport || isReport || fullSizeChartId ? undefined : barTopOffset;
Review Comment:
You're right that nested tab bars overlap on mobile, and thanks for reading
the interaction between the two paths carefully — but I don't think it belongs
in this PR.
That mobile rule predates this change: `git blame` puts it at 6211f9936b
(#37141, mobile consumption mode). Because it targets `.ant-tabs-nav` as a
descendant rather than a direct child, every nested bar already pins at `top:
0` on master today, with or without this PR. Nothing here makes it worse — the
only mobile-adjacent change is extracting `useIsMobile()` into a named variable
so it can be read twice. And it's reachable only with `MOBILE_CONSUMPTION_MODE`
enabled.
Propagating measured heights on mobile is also more than a parameter change.
On mobile the top-level tab bar is pinned by the media query and renders
outside the `StickyTabsOffsetContext.Provider`, which wraps only
`DashboardContainer` — so there's no measured root to stack from. A real fix
means measuring the top-level bar, seeding the context with that height instead
of `undefined` on mobile, and reworking the mobile rule to consume `stickyTop`
with a direct-child selector. That's a change to the mobile consumption
feature's own styling, and I'd rather not fold it into a desktop-nested-tabs PR
where it can't get the mobile review attention it deserves.
I'll open a follow-up issue for it and cc @rusackas, so the narrow-viewport
nested-tabs test you're asking for can land alongside the fix rather than ahead
of it.
---
🤖 _Drafted by Claude Code, co-signed by @sfirke._
--
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]
