-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
feat(index): add empty line to even code box size #8450
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?
feat(index): add empty line to even code box size #8450
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
👋 Codeowner Review RequestThe following codeowners have been identified for the changed files: Team reviewers: @nodejs/nodejs-website Please review the changes when you have a chance. Thank you! 🙏 |
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 addresses a variable height issue in code blocks by applying a fixed height of 19rem to code blocks within active tabs. The fix targets the BaseCodeBox component when rendered inside tabs with an active state.
- Introduces a fixed height constraint for code blocks in active tabs
- Uses CSS selector targeting
div[data-state='active']to scope the height change
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| div[data-state='active'] .root pre { | ||
| height: 19rem; | ||
| } |
Copilot
AI
Dec 25, 2025
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 CSS selector is overly broad and may cause unintended side effects. It targets any div with data-state='active' followed by any descendant with class 'root' and a pre element. This could affect other components in the codebase that use similar patterns. Consider using a more specific selector by adding a unique class to the CodeTabs or Tabs component wrapper, or scope this selector to be more specific to the BaseCodeBox component within a tabs context.
| } | ||
|
|
||
| div[data-state='active'] .root pre { | ||
| height: 19rem; |
Copilot
AI
Dec 25, 2025
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 fixed height of 19rem (304px) is a magic number without documentation or explanation. Consider extracting this value to a CSS custom property (CSS variable) with a descriptive name, or adding a comment explaining why this specific height was chosen. Additionally, this fixed height may cause issues on different screen sizes or with varying amounts of code content. Consider whether overflow-y: auto should be added to handle cases where content exceeds the fixed height, or whether a max-height with min-content behavior would be more appropriate.
| @apply size-4; | ||
| } | ||
|
|
||
| div[data-state='active'] .root pre { |
Copilot
AI
Dec 25, 2025
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.
Setting a fixed height on the pre element without overflow-y handling will cause content that exceeds 19rem to be cut off and inaccessible to users. The existing overflow-x-auto is on the nested code element (line 19), not the pre element. Consider adding overflow-y-auto to this rule to ensure users can scroll through longer code blocks, or verify that all code content in active tabs will always fit within 19rem.
| div[data-state='active'] .root pre { | |
| div[data-state='active'] .root pre { | |
| @apply overflow-y-auto; |
avivkeller
left a comment
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.
Don't we just need to make all the code boxes the same height, without CSS changes?
|
@avivkeller I used CSS because the height difference was coming from content length across tabs, and there wasn’t an existing prop or layout constraint to normalize it. When you say “without CSS changes,” do you mean setting a fixed height at the component level or something else? |
|
I mean just adding a line to the code block, surely there's a way we can get it to the 14? lines it needs to be |
|
Oh ok, then i will add an extra line to code block of Streams pipeline section, that ought to do it |
| ); | ||
|
|
||
| // run with `node streams.mjs` | ||
|
|
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.
Perhaps put the line before the comment?
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 have shifted it just above comment
|
Lighthouse Results
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8450 +/- ##
==========================================
+ Coverage 73.81% 73.83% +0.01%
==========================================
Files 109 109
Lines 9255 9255
Branches 313 313
==========================================
+ Hits 6832 6833 +1
+ Misses 2421 2420 -1
Partials 2 2 ☔ View full report in Codecov by Sentry. |
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 fix is so funny. Anyhow, SGTM
Description
This PR fixes the variable height issue of code block by introducing fixed height for the code block
Validation
Screen.Recording.2025-12-25.161204.mp4
Related Issues
Fixes #8425
Check List
pnpm formatto ensure the code follows the style guide.pnpm testto check if all tests are passing.pnpm buildto check if the website builds without errors.