fix: DC-5428 Update mobile tab component #7127
Conversation
Dangerous URL checkNo absolute URLs to prisma.io/docs found. |
WalkthroughImplements a collapsible, overflow-aware Tabs header with measurement/resize/overflow logic and chevron indicator; adds supporting SCSS selectors; adds a Font Awesome chevron-up mapping; overrides code background inside Mantine modal bodies; adds a trailing newline to an SCSS file. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Tabs as Tabs component
participant Document as document/body
participant Window as window
Note over Tabs: mount / init
Tabs->>Tabs: calculateWidth()
Tabs->>Tabs: checkForOverflow()
Tabs->>Window: add resize listener
User->>Tabs: click header button
Tabs->>Tabs: toggle open state
Tabs->>Document: toggle scroll lock (when open)
User->>Tabs: select tab item
Tabs->>Tabs: handleTabChange -> set active
Tabs->>Tabs: set open = false
Tabs->>Document: restore scroll
Window-->>Tabs: resize event
Tabs->>Tabs: calculateWidth()
Tabs->>Tabs: checkForOverflow()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
Redirect checkThis PR probably requires the following redirects to be added to static/_redirects:
|
Deploying docs with
|
| Latest commit: |
c011c14
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9c63bbee.docs-51g.pages.dev |
| Branch Preview URL: | https://fix-dc-5428-tab-mobile.docs-51g.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/theme/Tabs/index.tsx (2)
17-20: Magic numbers should be extracted as constantsThe width calculation uses several magic numbers (300, 0.75, 48) without explanation. These should be named constants with comments.
+ // Layout constants for tab width calculation + const SIDEBAR_WIDTH = 300; // Estimated sidebar width + const CONTENT_RATIO = 0.75; // Use 75% of available content width + const TAB_PADDING = 48; // Total horizontal padding for tabs + const calculateWidth = () => { if (typeof document === "undefined") return 0; - return ((document.body.getBoundingClientRect().width - 300) * 0.75) - 48; // 48 is padding, 0.75 is 75% of content + return ((document.body.getBoundingClientRect().width - SIDEBAR_WIDTH) * CONTENT_RATIO) - TAB_PADDING; };
22-25: Fix useEffect dependency warningThe useEffect hook has a dependency on
widthbut also callscheckForOverflowwhich useswidth. This could cause unnecessary re-renders.useEffect(() => { setWidth(calculateWidth()); checkForOverflow(); - }, [width]); + }, []); // Only run once on mount + + useEffect(() => { + checkForOverflow(); + }, [width]); // Run checkForOverflow when width changessrc/css/custom.css (1)
1775-1777: Remove the !important on modal code background; scope it to theme or increase selector specificityLocation: src/css/custom.css:1775-1776 — the rule (.mantine-Modal-body code { background-color: var(--mantine-color-gray-1) !important; }) forces the same background across themes; remove the !important and either scope the rule to a theme (e.g., :root[data-theme="light"] .mantine-Modal-body code { ... }) or use a more specific selector / per-theme variable so dark theme and user prefs aren't overridden.
src/theme/Tabs/styles.module.scss (1)
31-61: Hide mobile tab list from assistive tech when closed (add aria-hidden or use display:none)I inspected src/theme/Tabs/index.tsx and styles.module.scss — role="tablist", role="tab", aria-selected and tabIndex are present, but .ulContent is only hidden via opacity/z-index/pointer-events which does not remove it from the accessibility tree.
- Add aria-hidden={!(open)} to the
in src/theme/Tabs/index.tsx or toggle a class that uses display:none / visibility:hidden for the closed state at the mobile breakpoint (src/theme/Tabs/styles.module.scss).- Re-verify keyboard focus (non-visible tabs must be tabIndex=-1) and test with a screen reader.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/css/all.css(1 hunks)src/css/custom.css(1 hunks)src/pages/index.module.scss(1 hunks)src/theme/Tabs/index.tsx(4 hunks)src/theme/Tabs/styles.module.scss(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/theme/Tabs/index.tsx (1)
src/components/Icon.tsx (1)
Icon(14-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lost Pixel Bot
🔇 Additional comments (2)
src/pages/index.module.scss (1)
498-498: EOF formatting looks goodAdding the trailing newline is a best practice for SCSS files.
src/css/all.css (1)
795-798: Icon mapping looks goodThe Font Awesome chevron-up icon mapping follows the existing pattern correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/theme/Tabs/index.tsx (2)
36-43: Remove debug console.log and compute overflow off the element itself.Drop the log and prefer scrollWidth vs clientWidth to detect horizontal overflow.
- const checkForOverflow = () => { - if (ulvalues.current && width > 0) { - const contentWidth = ulvalues.current.getBoundingClientRect().width; - console.log(width - contentWidth, contentWidth, width); - setOverflowing(contentWidth > width); - } - }; + const checkForOverflow = () => { + const el = listRef.current; + if (!el) return; + // True if content wider than visible area + setOverflowing(el.scrollWidth > el.clientWidth); + };
95-118: Fix invalidstructure; move non-
- children out and make the toggle a real button (with ARIA).
A
must only contain
- children. The current header and wrapping
inside thebreak semantics and a11y. Also, the toggle should be a button with proper ARIA and a stable aria-controls id.
Apply this diff to restructure the markup, add ARIA, and keep styling intact:
@@ - <ul - role="tablist" - aria-orientation="horizontal" - className={clsx( - "tabs", - { - "tabs--block": block, - }, - className - )} - > - <span className={clsx(styles.display, overflowing && styles.overflow)} onClick={(e) => handleOpen(e)}> - <span>{selectedValue}</span> - <Icon icon={`fa-regular fa-chevron-${open ? "up" : "down"}`} size="inherit" /> - </span> - <div ref={ulvalues} className={clsx(styles.ulContent, overflowing && styles.overflow, open && styles.open)}> + <> + <button + className={clsx(styles.display, overflowing && styles.overflow)} + onClick={handleOpen} + aria-expanded={open} + aria-controls={contentIdRef.current} + type="button" + > + <span>{selectedValue}</span> + <Icon icon={`fa-regular fa-chevron-${open ? "up" : "down"}`} size="inherit" /> + </button> + <ul + role="tablist" + aria-orientation="horizontal" + id={contentIdRef.current} + ref={listRef} + className={clsx( + "tabs", + { "tabs--block": block }, + className, + styles.ulContent, + overflowing && styles.overflow, + open && styles.open + )} + aria-hidden={!open && overflowing} + > {tabValues.map(({ value, label, attributes }) => ( <li // TODO extract TabListItem role="tab" tabIndex={selectedValue === value ? 0 : -1} aria-selected={selectedValue === value} key={value} - ref={(tabControl) => tabRefs.push(tabControl)} + ref={(tabControl) => tabRefs.push(tabControl)} onKeyDown={handleKeydown} onClick={handleTabChange} {...attributes} className={clsx("tabs__item", styles.tabItem, attributes?.className, { [`tabs__item--active ${styles.activeTab}`]: selectedValue === value, })} > {label ?? value} </li> ))} - </div> - </ul> + </ul> + </>Add these hook refs near other state to support the changes:
- const [width, setWidth] = useState(0); - const ulvalues = useRef(null); + const listRef = useRef<HTMLUListElement | null>(null); + const contentIdRef = useRef(`tab-list-content-${Math.random().toString(36).slice(2)}`);
🧹 Nitpick comments (2)
src/theme/Tabs/index.tsx (2)
12-20: Simplify measurement logic; remove magic numbers and circular width effect.Current width math uses body-based heuristics and a [width]-dependent effect that re-sets width. Measure the actual list instead and re-check on resize/content changes.
- const [width, setWidth] = useState(0); - const ulvalues = useRef(null); - - const calculateWidth = () => { - if (typeof document === "undefined") return 0; - return ((document.body.getBoundingClientRect().width - 300) * 0.75) - 48; // 48 is padding, 0.75 is 75% of content - }; + const listRef = useRef<HTMLUListElement | null>(null); + const contentIdRef = useRef(`tab-list-content-${Math.random().toString(36).slice(2)}`); - useEffect(() => { - setWidth(calculateWidth()); - checkForOverflow(); - }, [width]); + // Initial check + useEffect(() => { + checkForOverflow(); + }, []); useEffect(() => { - const handleResize = () => { - const newWidth = calculateWidth(); - setWidth(newWidth); - }; + const handleResize = () => checkForOverflow(); window.addEventListener("resize", handleResize); return () => window.removeEventListener("resize", handleResize); }, []); + + // Re-check when content that can affect width changes + useEffect(() => { + checkForOverflow(); + }, [open, selectedValue, tabValues]);If content can resize independently (fonts, async icons), consider a ResizeObserver on listRef for robustness. I can add it if desired.
Also applies to: 22-35
77-81: Guard toggle when not overflowing.No need to open/close when everything fits; it reduces accidental toggles.
- const handleOpen = (event) => { - const list = event.currentTarget; - setOpen(!open); - blockElementScrollPositionUntilNextRender(list); - } + const handleOpen = (event) => { + if (!overflowing) return; + const list = event.currentTarget; + setOpen((v) => !v); + blockElementScrollPositionUntilNextRender(list); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/theme/Tabs/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/theme/Tabs/index.tsx (1)
src/components/Icon.tsx (1)
Icon(14-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check internal links
- GitHub Check: runner / linkspector
- GitHub Check: Lost Pixel
🔇 Additional comments (2)
src/theme/Tabs/index.tsx (2)
97-97: Chevron direction is correct.Up when open, down when closed. All good.
53-53: Close menu on tab selection — good UX.Collapsing after selection is the right call.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/theme/Tabs/index.tsx (1)
82-130: Fix invalid DOM structure: button/div inside a UL is not allowed (breaks a11y).A
must contain only
- children. Move the toggle button outside the list and make the list itself (not a wrapping div) the collapsible/measure element.
Apply this structural diff:
- return ( - <ul - role="tablist" - aria-orientation="horizontal" - className={clsx( - "tabs", - { - "tabs--block": block, - }, - className - )} - > - <button - className={clsx(styles.display, overflowing && styles.overflow)} - onClick={(e) => handleOpen(e)} - aria-expanded={open} - aria-controls="tab-list-content" - type="button" - > - <span>{selectedValue}</span> - <Icon icon={`fa-regular fa-chevron-${open ? "up" : "down"}`} size="inherit" /> - </button> - <div - id="tab-list-content" - ref={ulvalues} - className={clsx(styles.ulContent, overflowing && styles.overflow, open && styles.open)} - aria-hidden={!open && overflowing} - > - {tabValues.map(({ value, label, attributes }) => ( - <li - // TODO extract TabListItem - role="tab" - tabIndex={selectedValue === value ? 0 : -1} - aria-selected={selectedValue === value} - key={value} - ref={(tabControl) => tabRefs.push(tabControl)} - onKeyDown={handleKeydown} - onClick={handleTabChange} - {...attributes} - className={clsx("tabs__item", styles.tabItem, attributes?.className, { - [`tabs__item--active ${styles.activeTab}`]: selectedValue === value, - })} - > - {label ?? value} - </li> - ))} - </div> - </ul> - ); + return ( + <div className={clsx({ "tabs--block": block }, className)}> + <button + className={clsx(styles.display, overflowing && styles.overflow)} + onClick={(e) => handleOpen(e)} + aria-expanded={overflowing ? open : undefined} + aria-controls={contentId} + type="button" + disabled={!overflowing} + aria-hidden={!overflowing} + > + <span>{selectedLabel}</span> + <Icon icon={`fa-regular fa-chevron-${open ? "up" : "down"}`} size="inherit" /> + </button> + <ul + id={contentId} + ref={ulvalues} + role="tablist" + aria-orientation="horizontal" + className={clsx("tabs", styles.ulContent, overflowing && styles.overflow, open && styles.open)} + aria-hidden={overflowing ? !open : undefined} + > + {tabValues.map(({ value, label, attributes }) => ( + <li + // TODO extract TabListItem + role="tab" + tabIndex={selectedValue === value ? 0 : -1} + aria-selected={selectedValue === value} + key={value} + ref={(tabControl) => tabRefs.push(tabControl)} + onKeyDown={handleKeydown} + onClick={handleTabChange} + {...attributes} + className={clsx("tabs__item", styles.tabItem, attributes?.className, { + [`tabs__item--active ${styles.activeTab}`]: selectedValue === value, + })} + > + {label ?? value} + </li> + ))} + </ul> + </div> + );Note: This assumes
contentIdandselectedLabelexist (see follow-up comments).
♻️ Duplicate comments (3)
src/theme/Tabs/index.tsx (3)
94-104: LGTM: ARIA wiring for the collapsible control.The button uses aria-expanded and aria-controls, and the panel gets the matching id and aria-hidden. Nice.
Also applies to: 104-109
102-102: LGTM: Chevron direction now matches expanded state.Icon flips up when open, down when closed. Good.
36-41: Confirmed: debug logging removed.No stray console.log remains in overflow check. All good.
🧹 Nitpick comments (5)
src/theme/Tabs/index.tsx (5)
12-16: Refine refs/state naming and typing.Rename
ulvaluestolistRefand give it a concrete type to avoidanyand improve readability.- const ulvalues = useRef(null); + const listRef = useRef<HTMLUListElement | null>(null);(Remember to update all usages.)
76-80: Use functional state update when toggling.Prevents stale-closure bugs under rapid clicks.
- const handleOpen = (event) => { - const list = event.currentTarget; - setOpen(!open); - blockElementScrollPositionUntilNextRender(list); - } + const handleOpen = (event) => { + const el = event.currentTarget; + setOpen((v) => !v); + blockElementScrollPositionUntilNextRender(el); + }
94-103: Header button UX/a11y polish when not overflowing.Avoid a focusable control that does nothing; disable and hide it from AT when there’s no overflow.
- <button - className={clsx(styles.display, overflowing && styles.overflow)} - onClick={(e) => handleOpen(e)} - aria-expanded={open} - aria-controls="tab-list-content" - type="button" - > + <button + className={clsx(styles.display, overflowing && styles.overflow)} + onClick={(e) => handleOpen(e)} + aria-expanded={overflowing ? open : undefined} + aria-controls={contentId} + type="button" + disabled={!overflowing} + aria-hidden={!overflowing} + >
94-99: Make the controlled panel id unique per instance.Static id "tab-list-content" can collide with multiple Tabs on a page. Use React 18’s useId.
-import React, { cloneElement, useEffect, useRef, useState } from "react"; +import React, { cloneElement, useEffect, useRef, useState, useId } from "react"; ... - const { blockElementScrollPositionUntilNextRender } = useScrollPositionBlocker(); + const { blockElementScrollPositionUntilNextRender } = useScrollPositionBlocker(); + const contentId = useId();And reference
{contentId}where the id/aria-controls are used.Also applies to: 104-106
55-75: Keyboard: also activate tab on Space.Per ARIA Authoring Practices, Space on a focused tab should activate it.
const handleKeydown = (event) => { let focusElement = null; switch (event.key) { case "Enter": { handleTabChange(event); break; } + case " ": // Space + case "Spacebar": { + handleTabChange(event); + break; + } case "ArrowRight": { const nextTab = tabRefs.indexOf(event.currentTarget) + 1; focusElement = tabRefs[nextTab] ?? tabRefs[0]; break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/theme/Tabs/index.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/theme/Tabs/index.tsx (1)
src/components/Icon.tsx (1)
Icon(14-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Check internal links
- GitHub Check: runner / linkspector
- GitHub Check: Lost Pixel
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
src/theme/Tabs/index.tsx (1)
101-101: Show the selected tab label in the header, not the raw value.Value may be an internal id; label is user-facing.
[suggest_minor_issue]- <span>{selectedValue}</span> + <span>{selectedLabel}</span>Add once near the top of TabList:
+ const selectedLabel = tabValues.find((t) => t.value === selectedValue)?.label ?? selectedValue;
carlagn
requested review from
Techboidesign,
aidankmcalister,
ankur-arch,
nikolasburk and
nurul3101
Update text primary Paddings
carlagn
dismissed stale reviews from coderabbitai[bot] and mhessdev
via
c011c14
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/theme/Tabs/styles.module.scss (3)
6-32: Tighten .display: drop duplicate font-weight and add a keyboard focus style
- font-weight is declared twice.
- Add a visible :focus-visible style for keyboard users; the element is clickable (cursor: pointer).
.display { display: flex; justify-content: space-between; cursor: pointer; border-radius: 8px; align-items: center; - outline: 1px solid var(--border-color); - border: none; + border: 1px solid var(--border-color); width: 100%; padding: 6px 12px; margin-bottom: 4px; background: var(--surface-brand-grey-strong); color: var(--primary-font-color); font-family: Inter; font-style: normal; font-weight: 600; line-height: 24px; - font-weight: 600; white-space: nowrap; font-size: 0.875rem; + &:focus-visible { + outline: 2px solid var(--focus-color, var(--primary-font-color)); + outline-offset: 2px; + } @media (min-width: 940px) { display: none; &.overflow { display: flex; } } }
45-53: Reduce duplication between .ulContent (mobile) and .ulContent.overflowBoth blocks set nearly identical “hidden” styles. Consider consolidating into a single “.collapsed” class or reusing the same declarations via a placeholder/mixin to avoid drift.
67-76: Conflicting width rules — last one wins.tabList sets width: fit-content; then width: 100%. Keep only the intended one to avoid confusion.
.tabList { margin-bottom: var(--ifm-leading); - width: fit-content; + /* width: fit-content; */ /* removed in favor of full-width layout */ margin: 1rem 0; max-width: 100%; - width: 100%; + width: 100%; > ul { overflow: unset; position: relative; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/theme/Tabs/styles.module.scss(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: runner / linkspector
- GitHub Check: Check internal links
- GitHub Check: Lost Pixel
🔇 Additional comments (3)
src/theme/Tabs/styles.module.scss (3)
115-116: Good call on preventing label wrappingnowrap improves tab header stability.
140-147: Active tab color update looks rightAligns with primary font color and improves contrast.
92-94: Confirm :has() support for our targets or add a fallbackChromium (Chrome/Edge/Opera) supports :has() since Chrome 105 and Safari since 15.4; Firefox desktop gained support in 121+, so Firefox ESR 115 does NOT support :has() (newer ESRs based on FF 128/140 do). If we must support ESR115/older Firefox, avoid relying on :has() for critical styling — add a JS fallback/polyfill or alternative CSS.
Location: src/theme/Tabs/styles.module.scss lines 92–94 (also 102–106).
3 participants
Fixes #DC-5428
Summary by CodeRabbit
New Features
Style
Chores