Refactor hover widget styles for improved layout and spacing #276702
Conversation
mrleemurray
added
ux
There was a problem hiding this comment.
Pull Request Overview
This PR refactors hover widget styles to improve layout consistency and visual alignment. The changes consolidate hover styling by moving specific SCM history item hover styles into general hover widget styles and adjusting spacing and sizing throughout.
Key Changes
- Removes
line-height: 19pxfrom workbench hover for more flexible line height handling - Consolidates codicon sizing to 16px across hover content paragraphs
- Adjusts margin values for
<hr>,<h1-h6>, and paragraph elements to reduce spacing - Removes redundant SCM-specific history item hover styles that are now covered by base hover styles
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/vs/workbench/contrib/scm/browser/media/scm.css |
Removes redundant history-item-hover paragraph and span styling that duplicates base hover styles |
src/vs/platform/hover/browser/hover.css |
Adds codicon sizing rules and flexbox layout for hover content paragraphs |
src/vs/base/browser/ui/hover/hoverWidget.css |
Adjusts margins for headings, hr elements, and paragraphs; adds flexbox layout and image styles |
| font-size: 12px; | ||
| } | ||
|
|
||
| .monaco-hover.workbench-hover.compact .codicon, |
There was a problem hiding this comment.
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.
| .monaco-hover.workbench-hover.compact .codicon, | |
| .monaco-hover.workbench-hover.compact .codicon { | |
| 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; |
There was a problem hiding this comment.
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.
| /* 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; |
| margin-top: 4px; | ||
| margin-bottom: -4px; | ||
| margin-top: 0px; | ||
| margin-bottom: 0px; |
There was a problem hiding this comment.
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.
| margin-bottom: 0px; | |
| margin-bottom: 0; |
| */ | ||
| .monaco-hover .markdown-hover .hover-contents:not(.code-hover-contents):not(.html-hover-contents) span.codicon { | ||
| margin-bottom: 2px; | ||
| /* margin-bottom: 2px; */ |
There was a problem hiding this comment.
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.
| /* margin-bottom: 2px; */ |
| .monaco-hover p:first-child { | ||
| margin-top: 4px; | ||
| display: flex; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
[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.
| font-size: 16px !important; | ||
| } | ||
|
|
||
| .monaco-hover .monaco-hover-content p:last-child { |
There was a problem hiding this comment.
[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.
| .monaco-hover .monaco-hover-content p:last-child { | |
| .monaco-hover .monaco-hover-content p:last-child:has(a) { |
Tyriar
left a comment
Tyriar
left a comment
There was a problem hiding this comment.
The list item spacing looked a lot better before imo with less padding.
Also a good idea to test many of the hovers, especially more detailed ones:
- Editor tabs
- Extension view
- Terminal tabs
- GHPR Issues and PRs
- Manage language model editor list item hovers (just fixed a margin issue here)
|
cc @benibenj |
|
Looks like Otherwise new looks 👌 |
|
Thanks @mrleemurray.
|
Improve the layout and spacing of the hover widget by refining styles and removing unused CSS rules. Adjustments enhance the overall appearance & align codicon sizes with the rest of the UI.