-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
[Docs] Improve docs search experience by limiting code block height in search results #21853
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
Conversation
Signed-off-by: mgoin <[email protected]>
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.
Code Review
This pull request improves the search experience by limiting the height of code blocks in search results. My review provides feedback to further enhance this by improving code readability, ensuring cross-browser compatibility for the new custom scrollbars, and increasing CSS maintainability. I've consolidated these points into a single comprehensive suggestion.
.md-search-result__article pre, | ||
.md-search-result__article .highlight { | ||
max-height: 8rem; | ||
overflow-y: auto; | ||
overflow-x: hidden; | ||
border-radius: 0.2rem; | ||
margin: 0.5rem 0; | ||
} | ||
|
||
/* Add fade-out gradient for truncated code blocks */ | ||
.md-search-result__article pre::after, | ||
.md-search-result__article .highlight::after { | ||
content: ""; | ||
position: absolute; | ||
bottom: 0; | ||
left: 0; | ||
right: 0; | ||
height: 1rem; | ||
background: linear-gradient(transparent, var(--md-default-bg-color)); | ||
pointer-events: none; | ||
} | ||
|
||
/* Position container for gradient overlay */ | ||
.md-search-result__article pre, | ||
.md-search-result__article .highlight { | ||
position: relative; | ||
} | ||
|
||
/* Truncate very long single lines in search results */ | ||
.md-search-result__article code { | ||
max-width: 100%; | ||
overflow-wrap: break-word; | ||
word-break: break-all; | ||
} |
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.
There are a few areas for improvement in this new CSS for search results:
- Code Readability: Using
word-break: break-all
can make code snippets unreadable by breaking identifiers and keywords. It's better to useoverflow-x: auto
to allow horizontal scrolling, which is a standard practice for code blocks and preserves readability. - Maintainability: The selector
.md-search-result__article pre, .md-search-result__article .highlight
is defined in two separate rules (lines 189-196 and 212-215). These should be combined into a single rule to improve maintainability. - Cross-browser Compatibility: The custom scrollbar styles (lines 230-251) are WebKit-specific. Support for Firefox should be added using the standard
scrollbar-width
andscrollbar-color
properties for a consistent user experience.
Here is a suggested implementation that addresses all these points. It combines the duplicated rules, improves code readability by enabling horizontal scroll, and adds Firefox scrollbar support.
.md-search-result__article pre,
.md-search-result__article .highlight {
position: relative;
max-height: 8rem;
overflow-y: auto;
overflow-x: auto;
border-radius: 0.2rem;
margin: 0.5rem 0;
scrollbar-width: thin;
scrollbar-color: var(--md-default-fg-color--light) var(--md-default-fg-color--lightest);
}
/* Add fade-out gradient for truncated code blocks */
.md-search-result__article pre::after,
.md-search-result__article .highlight::after {
content: "";
position: absolute;
bottom: 0;
left: 0;
right: 0;
height: 1rem;
background: linear-gradient(transparent, var(--md-default-bg-color));
pointer-events: none;
}
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
Another solution to this is to lower the search rank of large code blocks which we don't want to appear too near the top of search. (We already do this for the API docs) Would this be something that could work instead? Or would you prefer to implement both solutions? |
For some reason RTD didn't pick this up and build it. I'm checking locally |
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 fixes the issue where large code blocks would dominate the search modal, making it hard to scan through multiple results.