amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4400660658
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,108 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // A page the user may not open is left out rather than shown and refused:
+ // the server answers 403 for it, so an entry here would only advertise a
+ // dead end. The page stays registered, so a direct link still explains
+ // itself rather than rendering nothing.
+ const contributed = pages.filter(
+ page => page.nav && !page.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
Review Comment:
Split this into the half with an unambiguous answer and the half that needs
a product call.
The ordering is fixed in f17429210a. Traced it to `ExtensionsLoader`
awaiting `Promise.all` over the remote entries: each bundle registers at module
scope as its script finishes, the registry is a `Map`, and the snapshot is
insertion order -- so across extensions it is script-load-completion order,
which is network timing. `withExtensionPages` now sorts by path. Path rather
than label because the server refuses a second page claiming a path, so it is
unique and needs no tiebreak, and it is not translated, so the order does not
move with the locale.
The overflow half I have not touched, because the options differ in who they
affect. Measured the cliff at a few widths with the real cascade: entries start
wrapping at roughly 5 contributed at 1440px, 3 at 1280px, 2 at 1024px and 1 at
820px -- the host bar is already near-full at the narrow end with no extensions
at all. Nothing is displaced, since contributed entries are appended and
`StyledCol` wraps; the nav drops to a second row under the logo, which is
`z-index: 10`, so it paints over the page header. Below 768px `mode` flips to
`inline` and the problem disappears.
Two ways out:
- Cap the contributed entries and fold the remainder into one "More"
dropdown. `buildMenuItem` already renders an item with `childs` and no `url` as
a submenu, so this stays entirely inside `withExtensionPages` -- no host-chrome
change. What it needs is a number, and one number cannot be right at both 820px
and 1920px.
- Drop `disabledOverflow`. One line, but it puts the host's own entries
under antd's responsive measurement for the first time; nothing in the suite
asserts anything about that prop today, and `StyledMainNav` styles
`.ant-menu-submenu-horizontal` for a different thing than the rest item would
be. Since it came in with the antd 5 upgrade and was applied to every
horizontal menu at once, it reads deliberate -- do you remember the reasoning?
If it should come back on, that feels like its own PR with a visual check
rather than a rider on this one.
SIP-235 covers the ordering half -- "sorted by `order` then registration
order" -- with an `order` field this PR's `Page.nav` does not have, and is
silent on caps and overflow entirely. Adding `nav.order` is a public-API
addition I would rather settle on the SIP thread than here.
--
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]
