Skip to content

fix(jsref): nest member sub-pages in the JS reference sidebar#728

Open
caugner wants to merge 15 commits into
mainfrom
707-refine-Proxy-sidebar
Open

fix(jsref): nest member sub-pages in the JS reference sidebar#728
caugner wants to merge 15 commits into
mainfrom
707-refine-Proxy-sidebar

Conversation

@caugner

@caugner caugner commented May 26, 2026

Copy link
Copy Markdown
Contributor

Description

Generalize the JS reference sidebar so that any member entry (constructor, method, property) whose page has its own sub-pages renders those sub-pages nested below it, inside a <details> element with the parent's link in the <summary>. In practice this covers:

  • Proxy/Proxy/* handler traps now nest under the Proxy() constructor entry (which lives in the shared Proxy sidebar that all Proxy/* pages use).
  • Intl/Segmenter/segment/Segments (and Segments/containing, Segments/[Symbol.iterator]) now nest under the segment() instance method — previously they were surfaced as a separate sidebar via special cases.

Motivation

  • The previous Proxy/ProxyProxy/handler slug mapping pointed at a page that never existed, producing templ-redirected-link flaws across the Proxy reference pages.
  • Handler trap pages (Proxy/Proxy/apply, .get, .set, …) rendered under a separate, partially-broken sidebar instead of sharing the surrounding Proxy sidebar.
  • Existing one-off special cases (Proxy handler, Intl/Segmenter/segment/Segments) all want the same thing: surface sub-pages where they belong in the hierarchy. Doing it generically removes the special cases and avoids needing a new one each time another member grows sub-pages.

Additional details

  • JSRefItem now carries a sub_pages: HashMap<String, Vec<Page>> keyed by parent page slug. get_sub_pages for the class walks the whole sub-tree (depth None); depth-1 pages are bucketed by PageType as before, depth-≥2 pages go into sub_pages.
  • A new build_member_entry helper recursively constructs each entry, attaching MetaChildren::Children from sub_pages when present and setting Details::Closed so entries with children are collapsible and start closed (regardless of the surrounding group's open/closed state).
  • The unified (label, list) loop now also covers the Constructor group — there is no separate code path for it.
  • slug_to_object_name no longer has a Proxy/Proxy or Intl/Segmenter/segment/Segments special case — both fall through to the normal class resolution. GROUP_DATA loses the [Segments, Intl.Segmenter] entry. JSRefItem::from_obj_str loses its Segments-only title override.
  • The redundant ["Proxy", "Proxy/handler"] related-pages entry is removed.
  • test_slug_to_object_name gets assertions for Proxy, Proxy/Proxy, Proxy/Proxy/get, Intl/Segmenter/segment/Segments, and Intl/Segmenter/segment/Segments/containing.

Screenshots

Proxy page

Before After
image image

Proxy() constructor page

Before After
image image

handler.apply() trap page

Before After
image image

Intl.Segmenter page

Before After
image image

Intl.Segmenter.segment() page

Before After
image image

Intl.Segmenter.segment() -> Segments page

Before After
image image

Related issues and pull requests

Fixes #706.
Closes #732.

Supersedes #707.

All `Proxy/*` pages now share one sidebar with handler traps listed
under a new "Handler methods" group sourced from `Proxy/Proxy/*`
sub-pages. Drops the broken `Proxy/Proxy/*` → `Proxy/handler` slug
mapping (the page never existed) and the redundant `Proxy/handler`
related-pages entry.

Requires `Handler_methods` in `L10n-Common.json`.
@github-actions

github-actions Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

dd20530 was deployed to: https://rari-pr728.review.mdn.allizom.net/

caugner added 2 commits May 26, 2026 18:03
Rename the `pages` binding inside the `obj == "Proxy"` block to
`handler_pages` so it does not shadow the outer `pages` from the
generic sub-page fetch, making the scope obvious to readers.
Previously, any sub-page of `Proxy/Proxy` whose page type was not
`JavascriptInstanceMethod` would be silently dropped from the sidebar.
Emit a `tracing::warn!` instead so non-trap sub-pages (e.g. a future
data property) surface in build logs and can be triaged.
@caugner caugner changed the title feat(jsref): unify Proxy/* sidebar with Handler methods group fix(jsref): unify Proxy/* sidebar with Handler methods group May 26, 2026
@caugner
caugner marked this pull request as ready for review May 26, 2026 16:49
@caugner
caugner requested a review from a team as a code owner May 26, 2026 16:49
@caugner
caugner requested a review from LeoMcA May 26, 2026 16:49
@caugner

caugner commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@Josh-Cena Can you take a look at the screenshots and let me know if this looks good? 🙂

Handler methods are properties on a separate `handler` object passed
to the `Proxy()` constructor, not members of the `Proxy` class itself.
Move the group to the end of the sidebar so it reads as a distinct
concept rather than splitting the static-methods/static-properties
pairing.
@caugner
caugner force-pushed the 707-refine-Proxy-sidebar branch from b741af8 to 2ff51cb Compare May 26, 2026 17:05
@Josh-Cena

Copy link
Copy Markdown
Member

I'm not fond of this design. These methods should be under the constructor, not the class.

caugner added 2 commits May 27, 2026 11:05
Drop the separate `Handler methods` group at the class level and instead
render the traps as children of the `Proxy()` constructor entry, matching
their slug location (`Proxy/Proxy/*`).

This also removes the dependency on the `Handler_methods` L10n key.
Put the `Proxy()` link inside `<summary>` and the trap list below it,
so the entry can be collapsed/expanded like the surrounding groups.
caugner added 2 commits May 27, 2026 12:28
Drop the `handler_methods` field on `JSRefItem` and the corresponding
`if obj == \"Proxy\"` branch in `from_obj_str`. The fetch now lives in
`proxy_handler_trap_entries()`, called from the constructor-entry
builder where it's actually used. Keeps the per-class data struct free
of Proxy-specific quirks.
Generalize the Proxy-specific nesting into a depth-aware fetch and
recursive entry builder. For each class, `get_sub_pages` now walks the
whole sub-tree; depth-1 pages are still bucketed by `PageType`, and
depth-≥2 pages are kept in a `sub_pages` map keyed by parent slug.

A new `build_member_entry` helper recursively constructs sidebar entries
and nests sub-pages under their parent, using `<details>` whenever an
entry has children. This subsumes:

- `Proxy/Proxy/*` handler traps (now generic, not Proxy-specific).
- `Intl/Segmenter/segment/Segments` (and `Segments/containing`,
  `Segments/[Symbol.iterator]`), which previously appeared as a separate
  sidebar via special cases in `slug_to_object_name` and `GROUP_DATA`.
  Both special cases are removed.
@caugner caugner changed the title fix(jsref): unify Proxy/* sidebar with Handler methods group fix(jsref): nest member sub-pages in the JS reference sidebar May 27, 2026
`rsplit_once('/')` already yields the parent slug we need for the
nested-pages bucket; comparing it to `class_slug` tells us whether the
page is a direct member without an extra `strip_prefix` + `contains`
pass.
Top-level groups like Constructor or Instance methods still open by
default for the current class, but the nested `<details>` for a member
that has its own sub-pages (e.g. `Proxy()` with handler traps, or
`segment()` with `Segments`) now starts collapsed.
caugner added 3 commits May 27, 2026 15:41
The field holds descendants at depth >= 2 keyed by their immediate
parent slug, and `build_member_entry` walks it recursively — the prior
wording suggested it was specific to depth-1 members.
`get_sub_pages` only returns descendants of the class slug, so every
page here has a parent component. Replace the let-else `continue` with
an `expect` that documents the invariant.
Switching `get_sub_pages` from `depth: Some(1)` to `None` widens the
directory walk per class, but `read_sub_folders_internal` is
`#[memoize]`-cached, so the cost isn't paid per sidebar build. Call
that out so the next reader doesn't worry about a hot-path cost.
@caugner

caugner commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

I'm not fond of this design. These methods should be under the constructor, not the class.

Please have another look, I think this is what you're expecting.

@Josh-Cena

Copy link
Copy Markdown
Member

Yes, love this one! Thank you!

@caugner
caugner marked this pull request as ready for review May 27, 2026 18:32
@caugner

caugner commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

@mdn/content-team Do you have any concerns with this change?

@LeoMcA

LeoMcA commented May 28, 2026

Copy link
Copy Markdown
Member

I'll defer my code review until content have had a look

@caugner

caugner commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

ping @mdn/content-team

@Rumyra

Rumyra commented Jul 17, 2026

Copy link
Copy Markdown

Agree with @Josh-Cena - this is great - thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

jsref: generalize sidebar nesting for member pages with sub-pages Review jsref sidebar flaws

5 participants