Skip to content

Refactor hover widget styles for improved layout and spacing #276702

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.

Sign up for GitHub

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

Draft
mrleemurray wants to merge 3 commits into
base: main
Choose a base branch
from
+39 −19
Draft

Refactor hover widget styles for improved layout and spacing #276702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d0d7c1
Refactor hover widget styles for improved layout and spacing
mrleemurray Nov 10, 2025
1f015b3
Refactor hover styles for improved layout and spacing
mrleemurray Nov 10, 2025
943fb5f
Remove unused CSS rules for hover widget and adjust line height
mrleemurray Nov 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 20 additions & 6 deletions src/vs/base/browser/ui/hover/hoverWidget.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
.monaco-hover h4,
.monaco-hover h5,
.monaco-hover h6 {
margin: 8px 0;
margin: 4px 0;
}

.monaco-hover h1,
Expand All @@ -68,20 +68,33 @@
box-sizing: border-box;
border-left: 0px;
border-right: 0px;
margin-top: 4px;
margin-bottom: -4px;
margin-top: 0px;
margin-bottom: 0px;
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary use of units with zero values. Per CSS best practices, 0px should be written as 0 (no unit needed). This applies to both margin-top and margin-bottom.

Suggested change
margin-bottom: 0px;
margin-bottom: 0;

Copilot uses AI. Check for mistakes.
margin-left: -8px;
margin-right: -8px;
height: 1px;
}

.monaco-hover p:first-child,
.monaco-hover .code:first-child,
.monaco-hover ul:first-child {
margin-top: 0;
}

.monaco-hover p:last-child,
.monaco-hover p:first-child {
margin-top: 4px;
display: flex;
align-items: center;
}
Comment on lines +83 to +87
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The p:first-child selector now has display: flex which could affect all first paragraph elements in hovers, even those without images. The subsequent rule at line 89 specifically targets images inside first paragraphs. Consider whether the flexbox layout should only apply when an image is present, or add a comment explaining this is intentional for all first paragraphs.

Copilot uses AI. Check for mistakes.

.monaco-hover p:first-child img {
border-radius: 50%;
margin-right: 4px;
}

.monaco-hover p:last-child {
margin-bottom: 2px;
}

.monaco-hover .code:last-child,
.monaco-hover ul:last-child {
margin-bottom: 0;
Expand Down Expand Up @@ -199,7 +212,8 @@
* https://github.com/microsoft/vscode/issues/221359
*/
.monaco-hover .markdown-hover .hover-contents:not(.code-hover-contents):not(.html-hover-contents) span.codicon {
margin-bottom: 2px;
/* margin-bottom: 2px; */
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented-out code should be removed rather than left in the codebase. If this margin-bottom is no longer needed, delete this line entirely. If there's uncertainty about removing it, document the reasoning in a proper code comment.

Suggested change
/* margin-bottom: 2px; */

Copilot uses AI. Check for mistakes.
margin-right: 4px;
}

.monaco-hover-content .action-container a {
Expand Down
20 changes: 19 additions & 1 deletion src/vs/platform/hover/browser/hover.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
.monaco-hover.workbench-hover {
position: relative;
font-size: 13px;
line-height: 19px;
/* Must be higher than sash's z-index and terminal canvases */
z-index: 40;
overflow: hidden;
Expand All @@ -34,6 +33,25 @@
font-size: 12px;
}

.monaco-hover.workbench-hover.compact .codicon,
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CSS selector combines two unrelated rules. The .monaco-hover.workbench-hover.compact .codicon selector targets codicons in compact hovers, while .monaco-hover.monaco-hover-content .code-hover-contents targets code hover contents. These should be separate rules for clarity and maintainability. Split this into two rules to avoid confusion about which properties apply to which elements.

Suggested change
.monaco-hover.workbench-hover.compact .codicon,
.monaco-hover.workbench-hover.compact .codicon {
font-size: 16px;
}

Copilot uses AI. Check for mistakes.
.monaco-hover.monaco-hover-content .code-hover-contents {
font-size: 16px;
}

/* Increase codicon size in hover content paragraphs - use !important to override inline styles */
.monaco-hover .monaco-hover-content p .codicon {
font-size: 16px !important;
Comment on lines +41 to +43
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using !important to override inline styles is a code smell that suggests the underlying issue should be fixed at the source. The comment states this is to override inline styles, but it would be better to avoid setting inline styles for codicon font-size in the first place. Consider addressing the root cause instead of using !important.

Suggested change
/* Increase codicon size in hover content paragraphs - use !important to override inline styles */
.monaco-hover .monaco-hover-content p .codicon {
font-size: 16px !important;
/* Increase codicon size in hover content paragraphs */
.monaco-hover .monaco-hover-content p .codicon {
font-size: 16px;

Copilot uses AI. Check for mistakes.
}

.monaco-hover .monaco-hover-content p:last-child {
Copy link

Copilot AI Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Setting display: flex on all last paragraph elements without specific justification could cause layout issues. This appears to be specifically needed for the anchor element styling on lines 50-53, but will affect all last paragraphs regardless of whether they contain links. Consider a more specific selector like p:last-child:has(a) or document why all last paragraphs need flex display.

Suggested change
.monaco-hover .monaco-hover-content p:last-child {
.monaco-hover .monaco-hover-content p:last-child:has(a) {

Copilot uses AI. Check for mistakes.
display: flex;
}

.monaco-hover .monaco-hover-content p:last-child a {
display: inline-flex;
gap: 2px;
}

.monaco-hover.workbench-hover.compact .monaco-action-bar .action-item .codicon {
/* Given our font-size, adjust action icons accordingly */
width: 12px;
Expand Down
12 changes: 0 additions & 12 deletions src/vs/workbench/contrib/scm/browser/media/scm.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,18 +591,6 @@

/* History item hover */

.monaco-hover.history-item-hover p:first-child {
margin-top: 4px;
}

.monaco-hover.history-item-hover p:last-child {
margin-bottom: 2px !important;
}

.monaco-hover.history-item-hover p:last-child span:not(.codicon) {
padding: 2px 0;
}

.monaco-hover.history-item-hover hr {
margin-top: 4px;
margin-bottom: 4px;
Expand Down
Loading