style: fix outline style - use border instead of underline; make it dimmer #280
Conversation
✅ Deploy Preview for graypaper-reader ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
chmurson
changed the title
|
Warning Rate limit exceeded@chmurson has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request updates class names in src/components/Outline/Outline.tsx to change link styling from underline to bottom border, adjusting color and spacing classes by outline level. No logic, behavior, or exports were modified. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/components/Outline/Outline.tsx (5)
123-129: Anchor without href isn’t keyboard-focusable; add key handling and semantics.Using without href removes native keyboard focus/activation. At minimum, add tabIndex and onKeyDown for Enter/Space, or switch to a button with button semantics.
Apply this diff:
-function Link({ dest, children, className, onClick }: ILinkProps) { - return ( - <a onClick={() => dest && onClick(dest)} className={twMerge("cursor-pointer hover:opacity-75", className)}> - {children} - </a> - ); -} +function Link({ dest, children, className, onClick }: ILinkProps) { + const handleActivate = () => dest && onClick(dest); + const handleKeyDown: React.KeyboardEventHandler<HTMLAnchorElement> = (e) => { + if (e.key === "Enter" || e.key === " ") { + e.preventDefault(); + handleActivate(); + } + }; + return ( + <a + role="button" + tabIndex={0} + onClick={handleActivate} + onKeyDown={handleKeyDown} + className={twMerge( + "cursor-pointer hover:opacity-80 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-black/30 dark:focus-visible:ring-white/30 rounded-[2px] transition-colors", + className, + )} + > + {children} + </a> + ); +}If changing the element is acceptable, prefer a semantic button styled like a link.
69-70: Conflicting margin-top utilities; later mt-0.5 overrides mt-4.twMerge will keep the last mt-*; for firstLevel, mt-4 is effectively ignored. If that’s intended, remove mt-4 for clarity.
Apply this diff:
- <li key={item.title} className={twMerge(firstLevel ? "pl-0 mt-4" : "pl-4", "mt-0.5 first-of-type:mt-0")}> + <li key={item.title} className={twMerge(firstLevel ? "pl-0" : "pl-4", "mt-0.5 first-of-type:mt-0")}>
69-69: Key stability: item.title may collide; prefer a stable unique key.If two outline items share a title, React reconciliation can misbehave. Use dest (if serializable) or include index as a suffix.
Apply this diff:
- <li key={item.title} className={twMerge(firstLevel ? "pl-0" : "pl-4", "mt-0.5 first-of-type:mt-0")}> + <li + key={`${item.dest ?? item.title}-${index}`} + className={twMerge(firstLevel ? "pl-0" : "pl-4", "mt-0.5 first-of-type:mt-0")} + >Note: If dest can be an object, stringify or hash it first. --- `108-110`: **Hardcoded colors reduce theme consistency; consider using design tokens.** bg-[#eeeeee] and dark:bg-[#323232] bypass your brand palette and won’t respond to theme changes. Prefer semantic Tailwind tokens or CSS variables. Example (adjust to your tokens): ```diff - <div className="rounded-lg min-h-0 w-full py-6 px-6 bg-[#eeeeee] dark:bg-[#323232] overflow-y-auto"> + <div className="rounded-lg min-h-0 w-full py-6 px-6 bg-neutral-200 dark:bg-neutral-800 overflow-y-auto">Or define brand-surface variables and reference them via bg-[var(--surface)].
88-92: Border color opacity may be too low for WCAG on some displays.The /50 opacity on thin borders can be hard to perceive, especially in dark mode. Consider bumping to /60–/70 or increasing thickness on hover/focus.
If acceptable, tweak like:
- !firstLevel && - "dark:text-brand-light dark:border-brand-light/50 text-brand-dark border-brand-dark/50 mt-0.5", + !firstLevel && + "dark:text-brand-light dark:border-brand-light/60 text-brand-dark border-brand-dark/60 mt-0.5",To verify contrast against your palette, please confirm actual hex values in tailwind.config and test quickly in Storybook/screenshots.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/components/Outline/Outline.tsx(1 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). (1)
- GitHub Check: build
|
I like #281 better, I feel it's still too heavy. However I wonder if you could try border-bottom/text-underline only on the NON-numeric part of the chapter. If you have a spare minute could you introduce an extra |
Interesting request :) I've done as you suggested. Please double check 🙏 |
chmurson
force-pushed
the
outline-change--lighter-underline
branch
from
ce685be to
b2a9f9c
Compare
2 participants
What