[WEB-4686] Add PageHeader component and migrate LanguageSelector to Radix UI #2956
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a new utility strip feature by adding Radix UI Select-based components (PageHeader, Select wrapper, Skeleton), refactoring LanguageSelector to use the new Select component, simplifying Breadcrumbs logic, consolidating styling utilities, and adding IBM Plex Serif font support across the application layout. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LanguageSelector
participant Select as Radix Select
participant Navigate
participant UrlQuery as URL Query
User->>LanguageSelector: Open language dropdown
activate LanguageSelector
LanguageSelector->>Select: Render Select.Root (via wrapper)
Select->>LanguageSelector: Scroll unlock on open
LanguageSelector->>LanguageSelector: Display skeleton if loading
User->>Select: Select language
LanguageSelector->>LanguageSelector: Update value state
LanguageSelector->>LanguageSelector: track('language_changed')
LanguageSelector->>Navigate: navigate(path?lang=code)
Navigate->>UrlQuery: Update query parameter
deactivate LanguageSelector
sequenceDiagram
participant User
participant PageHeader
participant LayoutContext
participant Tooltip
participant LlmService as External LLM
Note over PageHeader: Component Renders
PageHeader->>LayoutContext: useLayoutContext (activePage)
LayoutContext-->>PageHeader: Receive product, page, language
PageHeader->>PageHeader: useMemo compute llmLinks
PageHeader->>PageHeader: Render title + description
alt activePage.languages.length > 0
PageHeader->>PageHeader: Render LanguageSelector
end
PageHeader->>PageHeader: Render LLM link buttons
User->>PageHeader: Hover on LLM link
PageHeader->>Tooltip: Show tooltip (model + login note)
User->>PageHeader: Click LLM link
PageHeader->>PageHeader: track('llm_link_clicked')
PageHeader->>LlmService: window.open(llmUrl, '_blank')
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
Comment |
jamiehenson
changed the title
jamiehenson
force-pushed
the
web-4684-docs-redesign
branch
from
c967a27 to
8aec4e5
Compare
jamiehenson
force-pushed
the
web-4686-docs-header
branch
2 times, most recently
from
fc79806 to
7e18063
Compare
jamiehenson
force-pushed
the
web-4686-docs-header
branch
from
7e18063 to
c68a56a
Compare
jamiehenson
force-pushed
the
web-4686-docs-header
branch
from
c68a56a to
e34cb26
Compare
|
|
||
| const Article: FC<{ children: React.ReactNode }> = ({ children }) => ( | ||
| <article className="flex-1 overflow-x-hidden relative z-10"> | ||
| <article className="flex-1 overflow-x-hidden px-4 -mx-4 relative z-10"> |
There was a problem hiding this comment.
This allows focus outlines to be visible when otherwise they would be cut off by the container's overflow property
| @@ -0,0 +1,12 @@ | |||
| import cn from '@ably/ui/core/utils/cn'; | |||
There was a problem hiding this comment.
Same as was in Header, just moved out so it could be used in the assets added here
| }, [activePage?.tree]); | ||
|
|
||
| if (breadcrumbNodes.length === 0) { | ||
| if (!activePage?.tree || activePage.tree.length === 0) { |
There was a problem hiding this comment.
Don't need to do this processing anymore, we no longer care about eliminating duplicates
| import cn from '@ably/ui/core/utils/cn'; | ||
|
|
||
| import { getRandomChannelName } from '../blocks/software/Code/get-random-channel-name'; | ||
| import Aside from '../blocks/dividers/Aside'; |
There was a problem hiding this comment.
Same imports, just reorganised into logical groups
| @@ -0,0 +1,13 @@ | |||
| import cn from '@ably/ui/core/utils/cn'; | |||
There was a problem hiding this comment.
Ripped from shadcn - https://ui.shadcn.com/docs/components/skeleton
| extend: { | ||
| ...ablyUIConfig.theme.extend, | ||
| fontFamily: { | ||
| serif: ['IBM Plex Serif', 'Georgia', 'Cambria', 'Times New Roman', 'Times', 'serif'], |
aralovelace
left a comment
aralovelace
left a comment
There was a problem hiding this comment.
Much nicer language selector!
One thing I notice when dropdown is active the right side nav moved as well? is it expected? or fixed on another PR?
| @@ -0,0 +1,13 @@ | |||
| import cn from '@ably/ui/core/utils/cn'; | |||
| import { useEffect, useMemo, useState } from 'react'; | ||
| import { useLocation } from '@reach/router'; | ||
| import Select from 'react-select'; | ||
| import * as Select from '@radix-ui/react-select'; |
jamiehenson
force-pushed
the
web-4686-docs-header
branch
from
c5ee88b to
78b153b
Compare
jamiehenson
had a problem deploying
to
ably-docs-web-4686-docs-7kp8gz
jamiehenson
force-pushed
the
web-4686-docs-header
branch
from
78b153b to
a01cb40
Compare
@aralovelace interesting one, I hadn't noticed this - you actually unearthed something undesirable about this radix select component that I've spent a bit of time getting my head around. For whatever reason, opening the dropdown locks scrolling on the page and there's no way to disable that - this is the thing that makes the right sidebar shift because when the scrolling is locked, the scrollbar is taken away. This is solvable via CSS, but the locking is the worst part as it's somewhat bizarre behaviour for a dropdown box to force. What I've done is repackaged Radix select in a local UI file that also includes a workaround that disables this scroll locking until Radix offers a way to turn it off. I briefly tried reverting back to react-select which doesn't have this quirk but it's a greater loss as it's a step back accessibility-wise. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
| @@ -0,0 +1,50 @@ | |||
| import { useLayoutEffect } from 'react'; | |||
There was a problem hiding this comment.
The TLDR of this file is, it reexports Radix, but bundles in a useEffect in with Root that forces scrolling to be unlocked as Radix itself insists on locking it
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Layout/utils/styles.ts (1)
9-12: Consider usingcnconsistently for all composite class strings.The
secondaryButtonClassNameis defined as a plain string literal, whileiconButtonClassNameusescnto compose classes. For consistency and to leveragecn's deduplication and conditional class handling, consider wrappingsecondaryButtonClassNameincnas well.-export const secondaryButtonClassName = - 'focus-base flex items-center justify-center gap-2 px-4 py-[7px] h-9 ui-text-label4 text-neutral-1300 dark:text-neutral-000 rounded border border-neutral-400 dark:border-neutral-900 hover:border-neutral-600 dark:hover:border-neutral-700'; +export const secondaryButtonClassName = cn( + 'focus-base flex items-center justify-center gap-2 px-4 py-[7px] h-9 ui-text-label4 text-neutral-1300 dark:text-neutral-000 rounded border border-neutral-400 dark:border-neutral-900 hover:border-neutral-600 dark:hover:border-neutral-700' +);src/components/Layout/LanguageSelector.tsx (1)
50-61: Query parameter handling may lose existing parameters.Line 59 replaces the entire query string with
?lang=${option.label}. If the URL contains other query parameters, they will be lost. Consider preserving existing parameters by parsing and updating the search params.const handleValueChange = (newValue: string) => { setValue(newValue); const option = options.find((opt) => opt.value === newValue); if (option) { track('language_selector_changed', { language: option.label, location: location.pathname, }); - navigate(`${location.pathname}?lang=${option.label}`); + const params = new URLSearchParams(location.search); + params.set('lang', option.label); + navigate(`${location.pathname}?${params.toString()}`); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
package.json(1 hunks)src/components/Article/index.tsx(1 hunks)src/components/Head.tsx(1 hunks)src/components/Layout/Breadcrumbs.test.tsx(1 hunks)src/components/Layout/Breadcrumbs.tsx(2 hunks)src/components/Layout/Header.tsx(1 hunks)src/components/Layout/LanguageSelector.test.tsx(6 hunks)src/components/Layout/LanguageSelector.tsx(3 hunks)src/components/Layout/Layout.tsx(1 hunks)src/components/Layout/MDXWrapper.tsx(4 hunks)src/components/Layout/mdx/PageHeader.tsx(1 hunks)src/components/Layout/utils/styles.ts(1 hunks)src/components/ui/Select.tsx(1 hunks)src/components/ui/Skeleton.tsx(1 hunks)tailwind.config.js(1 hunks)
🔇 Additional comments (17)
src/components/Article/index.tsx (1)
5-5: LGTM!The padding/negative margin technique prevents focus outlines from being clipped by overflow, and the pattern is consistent with similar changes in Layout.tsx.
src/components/Head.tsx (1)
30-30: Verify font weight requirements.The IBM Plex Serif addition aligns with the Tailwind config update. Note that only italic weights (400 and 700) are being loaded.
If you plan to use IBM Plex Serif in non-italic (roman) styles, update the URL to include those weights:
-href="https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,100..800;1,100..800&family=Manrope:[email protected]&family=Source+Code+Pro:wght@600&family=IBM+Plex+Serif:ital,wght@1,400;1,700&display=swap" +href="https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,100..800;1,100..800&family=Manrope:[email protected]&family=Source+Code+Pro:wght@600&family=IBM+Plex+Serif:ital,wght@0,400;0,700;1,400;1,700&display=swap"tailwind.config.js (1)
15-17: LGTM!The serif font family addition is well-structured with appropriate fallbacks and aligns with the font loading in Head.tsx.
src/components/Layout/Layout.tsx (1)
45-45: LGTM!The padding/negative margin addition is consistent with the Article component change and maintains the conditional overflow behavior.
src/components/Layout/Header.tsx (1)
16-16: LGTM!Centralizing styling constants in a shared module is good practice and reduces duplication. The runtime behavior remains identical.
src/components/ui/Skeleton.tsx (1)
1-13: LGTM!Clean skeleton component implementation following standard patterns. The
data-slotattribute provides a useful hook for testing and styling.package.json (1)
52-52: Version 2.2.6 is current and appropriate for this use case.The latest stable version of @radix-ui/react-select is 2.2.6, and no direct security vulnerabilities are recorded for this version. The known issues (#2706 on resetting optional Selects and #2192 on imports) do not affect this implementation—LanguageSelector does not expose reset functionality, and the custom wrapper properly re-exports Radix UI components. The addition is sound and ready.
src/components/Layout/MDXWrapper.tsx (2)
55-56: LGTM: Improved type safety for code block processing.The new
ElementPropstype provides clearer typing for the code block processing logic and makes the element property contract explicit.
124-129: LGTM: Stronger validation for code element className.The updated logic now requires
classNameto be truthy (not just present), which prevents processing when the className is an empty string. This is a good defensive improvement.src/components/Layout/Breadcrumbs.tsx (1)
8-9: LGTM: Centralized link styles improve maintainability.Extracting the link styles into a constant makes it easier to maintain consistent styling across all breadcrumb links.
src/components/Layout/mdx/PageHeader.tsx (2)
35-37: Nice use of semantic typography for the description.The italic serif font styling for the description provides good visual hierarchy and readability.
66-66: No issues found. Both icon names are available in @ably/ui 17.9.3.The icons
icon-tech-openaiandicon-tech-claude-monoare defined in the @ably/ui package atnode_modules/@ably/ui/core/icons/tech/. They exist as SVG assets and will load correctly at runtime without any failures.src/components/Layout/Breadcrumbs.test.tsx (1)
40-96: LGTM: Comprehensive test coverage for the refactored breadcrumb logic.The updated tests cover:
- Rendering of all tree nodes including hash-link nodes
- Correct href attributes
- Disabled states for last item and non-linked nodes
- Mobile visibility behavior
- Edge cases like non-linked current pages
This provides strong validation of the new breadcrumb rendering logic.
src/components/Layout/LanguageSelector.tsx (2)
71-153: LGTM: Clean Radix UI implementation with good accessibility.The Radix Select implementation includes:
- Proper ARIA labels for the trigger
- Keyboard navigation support via Radix primitives
- Portal rendering for proper layering
- Item indicators for selected state
- Responsive styling and proper focus states
14-14: Select wrapper properly implements the scroll-lock workaround.The verification confirms that
src/components/ui/Select.tsxcontains a fully implementeduseScrollUnlockhook that addresses the Radix Select scroll-locking issue. The wrapper correctly detects thedata-scroll-lockedattribute via MutationObserver and removes it to restore scrolling, preventing the layout shifts mentioned in the PR.src/components/Layout/LanguageSelector.test.tsx (2)
36-49: LGTM: Mock data structure updated to match implementation.The mock for
src/data/languagescorrectly provideslanguageDatawith string versions andlanguageInfowith label mappings, matching the expected structure in the refactored component.
81-161: LGTM: Comprehensive test coverage for Radix-based implementation.The updated tests properly cover:
- Async dropdown interactions with
waitFor- Combobox role-based selection for accessibility
- Escape key handling for dismissing the dropdown
- Language filtering based on
activePage.languages- Default language selection
- Navigation behavior with query parameters
The use of
waitForhandles the async nature of Radix UI components correctly.
jamiehenson
force-pushed
the
web-4686-docs-header
branch
from
a01cb40 to
b0e9d5e
Compare
Human preamble
This PR does two things.
Firstly, implements this (with the exception of the Markdown buttons, since the functionality is not yet available and the buttons are easily addable):
Secondly, it refactors the
LanguageSelectorcomponent to be based around Radix primitives versusreact-select. The motivation for this is that it's a) more syntactically inkeeping with other components that also use Radix as a foundation, b) it's lighter and more customisable thanreact-select. Not critical to the outcome of this PR but worthwhile doing whilst I was in here.Review link: https://ably-docs-web-4686-docs-7kp8gz.herokuapp.com/docs/chat/rooms
Acceptance criteria:
Summary
PageHeaderMDX component with title, description, language selector, and LLM integration linksLanguageSelectorfrom react-select to Radix UI primitives for better accessibility and consistencyChanges
PageHeader Component
src/components/Layout/mdx/PageHeader.tsxLanguageSelector Migration
@radix-ui/react-selectTypography
font-serifTesting
Notes
scrollIntoViewmock in tests for Radix Select compatibility with JSDOM🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]
Summary by CodeRabbit
Release Notes
New Features
UI/UX Improvements
Tests