feat: implement new LeftNav and RightNav components for improved navi… #659
Conversation
…gation - Introduced LeftNavDesktop and LeftNavMobile components to enhance the layout and navigation experience. - Added RightNav and RightNavMobile components for better accessibility and organization of content. - Integrated a new H1 component with TitleAction for enhanced header functionality. - Updated translation strings and styles for consistency across the application. - Removed the deprecated Contributors component to streamline the codebase.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @shhdgit, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors and enhances the application's navigation and content presentation. It introduces a modular approach to navigation with dedicated desktop and mobile components for both left and right navigation panels. A new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant improvements to the site's navigation by implementing new LeftNav and RightNav components, and enhances the page header with a new TitleAction component. The removal of the deprecated Contributors component is a good cleanup.
My review focuses on the new components. I've found a critical issue in RightNav.tsx related to an invalid hook call that will cause a runtime error. I've also identified a few areas for improvement, such as a missing "Edit this page" feature which was a regression, and some missing dependencies in React hooks.
Overall, this is a great feature addition. Please address the comments to ensure the new components are robust and error-free.
| const generateToc = (items: TableOfContent[], level = 0, activeId = "") => { | ||
| const theme = useTheme(); | ||
|
|
||
| return ( | ||
| <Typography | ||
| component="ul" | ||
| sx={{ | ||
| listStyle: "none", | ||
| padding: 0, | ||
| }} | ||
| > | ||
| {items.map((item) => { | ||
| const { url, title, items } = item; | ||
| const { label: newLabel, anchor: newAnchor } = transformCustomId( | ||
| title, | ||
| url | ||
| ); | ||
| const itemId = url?.replace(/^#/, "") || ""; | ||
| const isActive = itemId && itemId === activeId; | ||
|
|
||
| return ( | ||
| <Typography key={`${level}-${item.title}`} component="li"> | ||
| <Typography | ||
| component="a" | ||
| href={newAnchor?.replace(sliceVersionMark, "")} | ||
| sx={{ | ||
| display: "flex", | ||
| textDecoration: "none", | ||
| fontSize: "14px", | ||
| lineHeight: "1.25rem", | ||
| borderLeft: `1px solid transparent`, | ||
| paddingLeft: `${0.5 + 1 * level}rem`, | ||
| paddingTop: "0.25rem", | ||
| paddingBottom: "0.25rem", | ||
| fontWeight: isActive ? "700" : "400", | ||
| color: isActive ? theme.palette.website.f1 : "inherit", | ||
| "&:hover": { | ||
| color: theme.palette.website.f3, | ||
| borderLeft: `1px solid ${theme.palette.website.f3}`, | ||
| }, | ||
| }} | ||
| > | ||
| {removeHtmlTag(newLabel)} | ||
| </Typography> | ||
| {items && generateToc(items, level + 1, activeId)} | ||
| </Typography> | ||
| ); | ||
| })} | ||
| </Typography> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
The useTheme hook is being called inside generateToc, which is a regular JavaScript function, not a React component or a custom hook. This will cause a runtime error.
To fix this, generateToc should be refactored into a proper React component. Please replace the generateToc function with the TocItems component I've suggested below. After that, you'll need to update line 125 to call <TocItems items={toc} activeId={activeId} /> instead of generateToc(...).
const TocItems = ({
items,
level = 0,
activeId = "",
}: {
items: TableOfContent[];
level?: number;
activeId?: string;
}) => {
const theme = useTheme();
return (
<Typography
component="ul"
sx={{
listStyle: "none",
padding: 0,
}}
>
{items.map((item, index) => {
const { url, title, items: children } = item;
const { label: newLabel, anchor: newAnchor } = transformCustomId(
title,
url
);
const itemId = url?.replace(/^#/, "") || "";
const isActive = itemId && itemId === activeId;
return (
<Typography key={`${level}-${item.title}-${index}`} component="li">
<Typography
component="a"
href={newAnchor?.replace(sliceVersionMark, "")}
sx={{
display: "flex",
textDecoration: "none",
fontSize: "14px",
lineHeight: "1.25rem",
borderLeft: `1px solid transparent`,
paddingLeft: `${0.5 + 1 * level}rem`,
paddingTop: "0.25rem",
paddingBottom: "0.25rem",
fontWeight: isActive ? "700" : "400",
color: isActive ? theme.palette.website.f1 : "inherit",
"&:hover": {
color: theme.palette.website.f3,
borderLeft: `1px solid ${theme.palette.website.f3}`,
},
}}
>
{removeHtmlTag(newLabel)}
</Typography>
{children && (
<TocItems items={children} level={level + 1} activeId={activeId} />
)}
</Typography>
);
})}
</Typography>
);
};
| {feedbackUrl && ( | ||
| <MenuItem | ||
| component="a" | ||
| href={feedbackUrl} | ||
| target="_blank" | ||
| rel="noreferrer" | ||
| onClick={handleContributeClose} | ||
| disableRipple | ||
| sx={{ | ||
| textDecoration: "none", | ||
| }} | ||
| > | ||
| <Typography component="span" color={theme.palette.carbon[900]}> | ||
| {t("doc.feedback")} | ||
| </Typography> | ||
| </MenuItem> | ||
| )} |
There was a problem hiding this comment.
This "Contribute" menu is missing the "Edit this page" option, which was available in the previous implementation for dev branches. This is a useful feature for contributors and its removal seems like a regression. Please consider adding it back to the menu. I've also included the logic to generate the edit URL.
{pathConfig.version === "dev" && (
<MenuItem
component="a"
href={`https://github.com/${getRepoFromPathCfg(
pathConfig
)}/edit/${pathConfig.branch}/${filePath}`}
target="_blank"
rel="noreferrer"
onClick={handleContributeClose}
disableRipple
sx={{
textDecoration: "none",
}}
>
<Typography component="span" color={theme.palette.carbon[900]}>
{t("doc.improve")}
</Typography>
</MenuItem>
)}
{feedbackUrl && (
<MenuItem
component="a"
href={feedbackUrl}
target="_blank"
rel="noreferrer"
onClick={handleContributeClose}
disableRipple
sx={{
textDecoration: "none",
}}
>
<Typography component="span" color={theme.palette.carbon[900]}>
{t("doc.feedback")}
</Typography>
</MenuItem>
)}
| setTimeout(() => { | ||
| // Observe all heading elements | ||
| headingIds.forEach((id) => { | ||
| const element = document.getElementById(id); | ||
| if (element) { | ||
| observer.observe(element); | ||
| } | ||
| }); | ||
| }, 1000); |
There was a problem hiding this comment.
The setTimeout with a 1000ms delay is a "magic number" and might be brittle, as it's not guaranteed that the elements will be ready after exactly 1 second on all connections or devices. Consider adding a comment to explain why this delay is necessary (e.g., waiting for MDX content to render) for future maintainability.
| export function RightNavMobile(props: RightNavProps) { | ||
| const { toc = [], pathConfig, filePath, buildType } = props; |
There was a problem hiding this comment.
The RightNavMobile component receives pathConfig, filePath, and buildType as props, but they are not used. These unused props should be removed to keep the component signature clean. You'll also need to remove them from the call site in src/templates/DocTemplate.tsx.
export function RightNavMobile(props: { toc?: TableOfContent[] }) {
const { toc = [] } = props;
| {generateMobileTocList(toc).map((item) => { | ||
| return ( | ||
| <MenuItem | ||
| key={`${item.depth}-${item.label}`} | ||
| onClick={handleClose} | ||
| dense | ||
| sx={{ | ||
| width: "calc(100vw - 2rem)", | ||
| }} | ||
| > | ||
| <Typography | ||
| component="a" | ||
| href={item.anchor} | ||
| sx={{ | ||
| width: "100%", | ||
| textDecoration: "none", | ||
| paddingLeft: `${0.5 + 1 * item.depth}rem`, | ||
| }} | ||
| > | ||
| {item.label} | ||
| </Typography> | ||
| </MenuItem> | ||
| ); | ||
| })} |
There was a problem hiding this comment.
The key for this MenuItem is ${item.depth}-${item.label}. If there are multiple items with the same label at the same depth, this will result in non-unique keys, which can cause rendering issues in React. Using the index from the map function would guarantee uniqueness.
{generateMobileTocList(toc).map((item, index) => {
return (
<MenuItem
key={`${item.depth}-${item.label}-${index}`}
onClick={handleClose}
dense
sx={{
width: "calc(100vw - 2rem)",
}}
>
<Typography
component="a"
href={item.anchor}
sx={{
width: "100%",
textDecoration: "none",
paddingLeft: `${0.5 + 1 * item.depth}rem`,
}}
>
{item.label}
</Typography>
</MenuItem>
);
})}
| const pageType = React.useMemo( | ||
| () => getPageType(language, pageUrl), | ||
| [pageUrl] | ||
| ); |
There was a problem hiding this comment.
The useMemo hook for calculating pageType uses the language variable, but it's missing from the dependency array. This could lead to stale data if the language changes.
| const pageType = React.useMemo( | |
| () => getPageType(language, pageUrl), | |
| [pageUrl] | |
| ); | |
| const pageType = React.useMemo( | |
| () => getPageType(language, pageUrl), | |
| [language, pageUrl] | |
| ); |
| const H1WithProps = React.useCallback( | ||
| (props: { children: React.ReactNode }) => ( | ||
| <H1 | ||
| pathConfig={pathConfig} | ||
| filePath={filePath} | ||
| pageUrl={pageUrl} | ||
| buildType={buildType} | ||
| language={language} | ||
| {...props} | ||
| /> | ||
| ), | ||
| [pathConfig, filePath, pageUrl] | ||
| ); |
There was a problem hiding this comment.
The useCallback hook for H1WithProps is missing buildType and language in its dependency array. These props are passed to the H1 component inside the callback, so they should be included to ensure the callback is updated when they change.
| const H1WithProps = React.useCallback( | |
| (props: { children: React.ReactNode }) => ( | |
| <H1 | |
| pathConfig={pathConfig} | |
| filePath={filePath} | |
| pageUrl={pageUrl} | |
| buildType={buildType} | |
| language={language} | |
| {...props} | |
| /> | |
| ), | |
| [pathConfig, filePath, pageUrl] | |
| ); | |
| const H1WithProps = React.useCallback( | |
| (props: { children: React.ReactNode }) => ( | |
| <H1 | |
| pathConfig={pathConfig} | |
| filePath={filePath} | |
| pageUrl={pageUrl} | |
| buildType={buildType} | |
| language={language} | |
| {...props} | |
| /> | |
| ), | |
| [pathConfig, filePath, pageUrl, buildType, language] | |
| ); |
- Introduced a new feature that generates a GitHub edit link for the current document, allowing users to easily contribute improvements. - The link is conditionally rendered based on the presence of pathConfig and filePath, enhancing user engagement with documentation.
- Changed the feedback string from "Request docs changes" to "Report a doc issue" in both English and Japanese translation files for improved clarity and user understanding.
- Added flexWrap property to the TitleAction component to enhance layout responsiveness. - Changed button text from "Copy Markdown for LLM" to "Copy for LLM" for brevity and clarity.
…gation