-
Notifications
You must be signed in to change notification settings - Fork 4k
Restyle the toolbar in CR and revision preview links #3631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Restyle the toolbar in CR and revision preview links #3631
Conversation
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR completely overhauls the admin toolbar displayed on change request (CR) and revision preview links with an enhanced visual design, animations, and improved functionality. The toolbar now features an animated GitBook logo, interactive magnification effects, and better responsive behavior.
- Replaces the simple static toolbar with an animated, interactive version featuring a prominently displayed GitBook logo
- Adds comprehensive animation system using framer-motion for smooth transitions, staggered appearances, and hover effects
- Implements a sophisticated magnification effect for toolbar buttons that scales and repositions them based on mouse proximity
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
DateRelative.tsx | Wraps the time element with a Tooltip component to improve UX |
useMagnificationEffect.ts | New custom hook implementing Mac-style dock magnification for toolbar buttons |
transitions.ts | Defines animation configurations and timing for toolbar elements |
index.ts | Updates exports to include new toolbar components |
Toolbar.tsx | Complete rewrite of toolbar components with animations and interactive features |
RefreshChangeRequestButton.tsx | Updates button to use new toolbar design system with motion values |
AnimatedLogo.tsx | New component for the animated GitBook logo with SVG path animation |
AnimatedLogo.module.css | CSS animations for the logo's segmented stroke effect |
AdminToolbarClient.tsx | New client component handling toolbar rendering logic |
AdminToolbar.tsx | Simplified server component that passes data to client component |
package.json | Adds motion library dependency for animations |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const container = containerRef.current; | ||
if (!container) return; | ||
|
||
const buttons = Array.from(container.querySelectorAll('#toolbar-button')) as HTMLElement[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using ID selectors for querying multiple elements violates HTML standards where IDs should be unique. Consider using a class selector like '.toolbar-button' instead of '#toolbar-button' to avoid potential conflicts and improve maintainability.
Copilot uses AI. Check for mistakes.
const buttons = Array.from( | ||
container.querySelectorAll('#toolbar-button') | ||
) as HTMLElement[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate use of the same problematic ID selector. This query is repeated from line 133, suggesting the need for a consistent approach using class selectors instead.
Copilot uses AI. Check for mistakes.
<motion.a | ||
href={href} | ||
onClick={onClick} | ||
id="toolbar-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the same ID 'toolbar-button' on multiple elements violates HTML standards. IDs must be unique within a document. Use a class name instead, such as 'className="toolbar-button"'.
Copilot uses AI. Check for mistakes.
@keyframes traceFill { | ||
0% { | ||
fill: transparent; | ||
stroke:#46474c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded color value #46474c is used multiple times without being defined as a CSS custom property. Consider defining it as a variable like '--trace-stroke-color: #46474c;' for better maintainability.
Copilot uses AI. Check for mistakes.
} | ||
95% { | ||
fill: transparent; | ||
stroke:#46474c; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded color value #46474c is used multiple times without being defined as a CSS custom property. Consider defining it as a variable like '--trace-stroke-color: #46474c;' for better maintainability.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (!changeRequest) { | ||
return null; | ||
return <AdminToolbarClient context={serializableContext} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can get very big, we should restrict what gets serialized as much as possible here.
On gitbook it doubles the size of the html and rsc (and we're not even the one with the biggest ones out there)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for flagging! I reduced it to just the fields we need here, lmk if you feel better about that: dbaaf35
function ChangeRequestToolbar(props: AdminToolbarClientProps) { | ||
const { context } = props; | ||
const { space, changeRequest, site } = context; | ||
const reduceMotion = Boolean(useReducedMotion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need that, it can be automatic? https://motion.dev/docs/react-accessibility#automatic
changeRequestId={changeRequest.id} | ||
revisionId={changeRequest.revision} | ||
updatedAt={new Date(changeRequest.updatedAt).getTime()} | ||
key="refresh-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a key?
<ToolbarButton | ||
title="Comment in app" | ||
href={`${changeRequest.urls.app}~/comments`} | ||
key="comment-button" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need a key?
const [minified, setMinified] = React.useState(true); | ||
const [showToolbarControls, setShowToolbarControls] = React.useState(false); | ||
const [isReady, setIsReady] = React.useState(false); | ||
const reduceMotion = Boolean(useReducedMotion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same is it really needed with the automatic mode?
// Small delay to ensure everything is settled | ||
setTimeout(() => { | ||
setIsReady(true); | ||
}, 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is arbitrary, what do you mean here? It probably depends of your CPU so you are delaying users with good machine and doing it too early for users with bad machines. Not sure that's a good strategy to wait an arbitrary time like this. Bad smell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also you need to get the timeout and clear it in the useEffect.
// After toolbar appears, wait then show the full content | ||
React.useEffect(() => { | ||
if (isReady) { | ||
setTimeout(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here you need to clear the timeout in the cleanup func.
}} | ||
> | ||
{/* Logo with stroke segments animation in blue-tints */} | ||
<motion.div key="logo" layout={!reduceMotion}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why key?
6ca6239
to
dbaaf35
Compare
Details
This PR updates the styles a bit and adds some more functionality to the toolbar displayed on CR and revision preview links.
Will likely be iterated on design-wise, but that can come later. Also, it is not yet draggable, but that is also the intent for a follow-up PR.
Preview
Change-request preview link:
CleanShot.2025-09-08.at.15.25.22.mp4
Revision preview link:

Dark mode: