fix(reader): match frappe-ui docs chrome#726
Merged
Merged
Conversation
Closes #724. - TOC: every "On this page" link sits at pl-4 regardless of heading level, so the label lines up with the rail on pages that open with an h3. - Sidebar: white (surface-base) and borderless; navbar drops its bottom border too. The tab row keeps its rule, so the chrome still reads as one band instead of stacking hairlines. - No smooth scrolling: anchor jumps land instantly. - Mobile: the nav toggle is a floating "Menu" pill pinned bottom-right (frappe-ui Button outline/lg), and the sheet is a fixed 80dvh panel on surface-base. The ⌘K palette drops its keyboard hints below sm, where they only wrap into a cramped two-line block. - Buttons, dropdowns and menu rows use the frappe-ui recipes verbatim (Button size/variant classes, Menu content, ItemListRow); the tab picker's bg-[var(--surface-white)] was an undefined token, so that dropdown had no background at all. - Tree toggles use frappe-ui Tree's stroked chevron-right in a size-5 slot. - Hand-written SVGs drop to stroke-width 1.5, the width frappe-ui normalises lucide to — at 2 they read heavier than the same icon in the docs. - Drop the unused ghost_button / dropdown / icon_button macros rather than restyle markup nothing renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace the centred spinner with skeleton rows rendered inside the real List chrome, so the column tracks and row height are already correct when the data lands — no jump from a blank panel to a table. Bar widths vary per column kind so it reads as a table rather than a block of identical grey lines. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The docs navbar renders its search field as `hidden ... md:flex`, so on mobile the only search bar is the one inside MobileNavSheet — a frappe-ui TextInput (size lg, subtle, prefix slot) that filters the nav list. The reader had no equivalent, which is why the mobile search never matched. Add it to the bottom sheet with TextInput's exact lg recipe (h-10, rounded-md, ps-10 for the prefix, subtle border/bg/hover/focus chain), filtering the tree in place: a row survives if its own title matches or a descendant's does, and matched groups expand so the hit is visible. The header's magnifier still opens the full-text palette — that's a wiki feature the docs don't have. Also from the same source: - sheet/overlay transitions use the docs' cubic-bezier(0.32,0.72,0,1) at 300ms in / 200ms out; overlay is black/40; the Menu pill hides while the sheet is open, as MobileNavSheet's `v-if` does. - drag handle geometry (pt-2 pb-1, w-10, surface-gray-3). - palette gains the ⌘↵ "new tab" hint it already supported, and `esc` / `K` take the text-sm sizing CommandPalette gives them. - search trigger hover border is outline-elevation-2, not outline-gray-2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reader's icon macro had hand-pasted SVG paths, and most had drifted to Heroicons: the magnifier was a circle-arc (`a 7 7 0 11-14 0`) where lucide draws `circle r="8"` plus a stub, and menu / check / list / clipboard / chevron / file-text were all a different glyph from the one frappe-ui docs renders. Only command, the arrows, sun and moon-star happened to match. Generate them instead, through the table the tab icons already use, so the reader draws the same pack at the same 1.5 stroke as the SPA and the docs: add a CHROME_ICONS list to generate-public-lucide.mjs and have render_icon resolve those names via lucide_svg. Brand marks (GitHub, Discord, X, …) stay inline — lucide dropped its brand set, which is why the docs navbar hand-writes its own GitHub mark too. Inline Heroicons chevrons and ticks left in the navbar, mobile header and split button go through the macro as well. Also stop the macro's default w-4/h-4 from overriding a caller's size: `size-[18px]` on the sheet's search prefix was rendering 16px, because Tailwind emits w-*/h-* after an arbitrary size-*. It now only applies the default when the caller passed no size utility. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reading vitepress/components/Docs/OnThisPage.vue instead of guessing from the rendered DOM corrects an earlier overcorrection and turns up two more mismatches. The indent rule is `'pl-7': x.type == 'h3' && h2Exists` — h3s are indented only when the page also has an h2. Flattening every link (the earlier fix for the misaligned label) was right for the all-h3 page it was tested on and wrong everywhere else, since it threw away the hierarchy on mixed pages. Gate it on an h2 being present instead: the label stays aligned on all-h3 pages and h3s nest again when there is an h2 to nest under. Also: - the rail is a bare `border-l`, i.e. the default outline-gray-1, not outline-gray-2 — ours was drawing a step darker than the docs'. - the aside is w-[200px], not w-56 (224px). - the label carries no colour class and no transition; it inherits, and our body ink already resolves to the same value. Both halves of the component are updated — the Jinja render and the nav-store's SPA re-render — and verified to agree on an all-h3 page, a mixed page, and after client-side navigation between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bottom sheet reused the desktop tree verbatim — 28px rows at text-sm — where frappe-ui docs' MobileNavSheet gives its links h-11, rounded-md and text-md, a real touch target instead of a packed line. Add a `dense` flag to render_wiki_tree (desktop keeps h-7) and have the sheet render at touch density, hover/active landing on surface-gray-2 / ink-gray-8 like the docs'. Drop the sheet's three internal rules as well: the docs sheet has no dividers at all, its only bordered element is the search field. The header row, space switcher and navbar-link footer each carried one. The shadow needed nothing — sheet and pill already resolve to the same four-layer shadow-2xl and 20px top radius as the docs'. Verified by comparing computed styles side by side. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Removing it left a white tree on a white page with nothing to sit against. The current Frappe docs layout (ffdocsnew) separates the two with a hairline — a bare `border-r`, i.e. the default outline-gray-1 — so take that back, including the width animating to 0 on collapse so a collapsed sidebar leaves no stray line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The chrome's bottom hairline belongs to whichever row ends it. With tabs that's the tab row; without them nothing carried it, so the sidebar's border started in mid-air below a borderless navbar. Give the navbar the rule only when no tab row follows, so exactly one line runs across the window either way and the sidebar's border meets it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the prior hidden-tab filtering issue is fixed by limiting matches to visible subtrees. Reviews (3): Last reviewed commit: "fix(reader): a lone next link sits left,..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Every tab's subtree is in the DOM; only the active one is displayed. The filter walked all of them, so a query matching a page in an inactive tab kept navHasResults true while nothing visible survived — a blank drawer with the no-results line suppressed. Count and hide only within displayed subtrees. Clearing still spans every subtree, so switching tabs can't surface rows left hidden by an earlier query. Reported by Greptile on #726. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
Good catch — reproduced it before fixing. On a space with tabs, searching for a page that only exists in an inactive tab gave 0 visible rows and a suppressed no-results line, i.e. a blank drawer. Every tab's subtree is in the DOM; only the active one is displayed, and the filter walked all of them. Fixed in ca50e52: matching and result-counting now run only within displayed subtrees. Clearing still spans every subtree, so switching tabs can't surface rows left hidden by an earlier query. Verified on a tabbed space (match in hidden tab → no-results shown; match in active tab → row shown; no match → no-results) and on a space without tabs, where the tree is a direct child of the nav rather than wrapped in a tab container. |
On a first page there is no previous link, and the `ml-auto` shoved the only button against the right edge with nothing opposite it. frappe-ui docs' PrevNextBtns adds no margin utility for that case: `justify-between` already leaves a single child at the start. Drop it from both the Jinja render and the nav-store's SPA re-render. Verified server-side and after client-side navigation: a lone next button starts at the container's left edge, and with both links present they still sit at the two ends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #724.
Problem
The public reader drifted from the frappe-ui docs it's modelled on: mismatched borders, a TOC label that didn't line up, a cramped mobile drawer, and hand-copied SVGs that had quietly become Heroicons. Change request lists showed a bare spinner while loading.
Solution
Audited against the frappe-ui source (
vitepress/components/Docs/*and the component recipes), not guesswork.Chrome
Icons
TOC
OnThisPage.vue, so the "On this page" label lines up on all-h3 pages without losing hierarchy elsewhere.Mobile
MobileNavSheet.vue.Change requests
Also fixes two bugs found on the way: the mobile tab picker used an undefined
--surface-whitetoken so it rendered with no background, and the icon macro's default size silently overrode a caller'ssize-[18px].