Conversation
…, and some fixes in sidebar, toc and ocumentation page
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
thomasfr
left a comment
There was a problem hiding this comment.
Is this the last version you shows in mondays meeting? I think i am missing some files, or?
In the content folder, is it necessary and if yes, why is i necessary to have a subfolder v1.0.1?
| const handleMouseEnter = (e: React.MouseEvent<HTMLButtonElement>) => { | ||
| if (copyState !== "copied") { | ||
| e.currentTarget.style.backgroundColor = "var(--color-code-copy-hover-bg)" | ||
| } | ||
| } | ||
|
|
||
| const handleMouseLeave = (e: React.MouseEvent<HTMLButtonElement>) => { | ||
| if (copyState !== "copied") { | ||
| e.currentTarget.style.backgroundColor = "transparent" | ||
| } | ||
| } |
There was a problem hiding this comment.
why do this with JS, couldn't you have done this with css? just conditionally render classes needed to do this based on copy state
app/components/pager.tsx
Outdated
| * @param previous - Optional previous page link data with title and path. | ||
| * @param next - Optional next page link data with title and path. | ||
| */ | ||
| export function Pager({ previous, next }: PagerProps) { |
There was a problem hiding this comment.
why did you call this pager?
|
|
||
| export const DesktopSidebarPanel = ({ items, className }: { items: SidebarSection[]; className: string }) => ( | ||
| <div | ||
| className={`sticky top-[var(--header-height)] h-[calc(100vh-var(--header-height))] w-80 flex-col overflow-hidden bg-[var(--color-background)] p-4 ${className}`} |
app/components/table-of-content.tsx
Outdated
|
|
||
| return ( | ||
| <div> | ||
| <a |
There was a problem hiding this comment.
why is this a normal link?
app/components/theme-toggle.tsx
Outdated
| {theme === "light" && <IconButton aria-label="Switch to dark mode" name="Sun" onClick={toggle} />} | ||
| {theme === "dark" && <IconButton aria-label="Switch to light mode" name="Moon" onClick={toggle} />} |
There was a problem hiding this comment.
why two, why not one, extract the check into const isDarkTheme = theme === "dark" then render conditionally based on that, if you don't support the system theme obviously
app/env.server.ts
Outdated
| GITHUB_OWNER: z.string(), | ||
| GITHUB_REPO: z.string(), |
There was a problem hiding this comment.
both of these are optional, that's correct?
| export default flatRoutes({ | ||
| ignoredRouteFiles: ["**/*.test.{ts,tsx}"], | ||
| }) | ||
| export default [ |
There was a problem hiding this comment.
why did you manually define routes?
There was a problem hiding this comment.
I defined them manually so it is easier to follow the routes. I think this way is cleaner for the template
app/routes/documentation-layout.tsx
Outdated
| // TODO sidebarItems are used on 2 places, change this to not call getSidebarTree twice | ||
| const sidebarItems = createSidebarTree("v1.0.1") //TODO use the version what is selected from the dropdown, after implementing docs generation |
There was a problem hiding this comment.
create issues, don't just write TODO in the code
app/ui/breadcrumbs.tsx
Outdated
|
|
||
| if (href && !isActive) { | ||
| return ( | ||
| <a href={href} className={classes}> |
There was a problem hiding this comment.
a tags cause full document reload requests, why are you using them instead of ?
app/utils/create-sidebar-tree.ts
Outdated
| map.get(parent)?.documentationPages.push({ slug: p.slug, title: p.title }) | ||
| } | ||
|
|
||
| return [...map.values()].filter((n) => parentOf(n.slug) === version) |
There was a problem hiding this comment.
could've been a constant with a descriptive name, i have no idea what this is
| .replace(/`([^`]+)`/g, "$1") | ||
| .replace(/\*\*([^*]+)\*\*/g, "$1") | ||
| .replace(/\*([^*]+)\*/g, "$1") | ||
| .replace(/\[([^\]]+)\]\([^)]+\)/g, "$1") | ||
| .replace(/\{[^}]*\}/g, "") | ||
| .replace(/<\/?[^>]+(>|$)/g, "") |
There was a problem hiding this comment.
comments on what this does would go a long way
| @@ -0,0 +1,16 @@ | |||
| export function splitSlug(slug: string) { | |||
There was a problem hiding this comment.
this funciton has tests in tests folder, maybe another utils don't have but I created another issue to add missing tests
AlemTuzlak
left a comment
There was a problem hiding this comment.
small changes, all in all very good
removed "v1.0.1" |
|
@AlemTuzlak @thomasfr This one is ready for review |
thomasfr
left a comment
There was a problem hiding this comment.
Looks good, thanks. I havent looked into all the components and hooks though.
| export function ThemeToggle() { | ||
| const [theme, setTheme] = useState<"light" | "dark" | null>(null) | ||
|
|
||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Great job on using this hook
Description
Initial working UI that contains a sidebar, table of contents, and rendered styled .mdx files, theme toggle, etc.
Added reusable components like the sidebar, code block, etc.
Added reusable UI primitives like alert dialog, icon button, etc.
Remaining vitest tests will be included in #4, with additional improvements in that issue's PR.
recording-2025-08-21-13-38-45.webm