Skip to content
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

Polish explorer tooltip rendering #174085

Merged
merged 9 commits into from
Feb 14, 2023
Merged
Prev Previous commit
Address PR comments
  • Loading branch information
lramos15 committed Feb 14, 2023
commit ffefdf6a69b619e155b6fe964b1354eb9e9e9316
Original file line number Diff line number Diff line change
@@ -9,6 +9,14 @@
height: 100%;
}

.explorer-item-hover {
/* -- Must set important as hover overrides the cursor -- */
cursor: pointer !important;
padding-left: 6px;
height: 22px;
font-size: 13px;
}

.explorer-folders-view .monaco-list-row {
padding-left: 4px; /* align top level twistie with `Explorer` title label */
}
15 changes: 9 additions & 6 deletions src/vs/workbench/contrib/files/browser/views/explorerViewer.ts
Original file line number Diff line number Diff line change
@@ -299,8 +299,10 @@ export class FilesRenderer implements ICompressibleTreeRenderer<ExplorerItem, Fu
element = options.target.targetElements[0];
}

const child = element.children[0] as Element | undefined;
const childOfChild = child?.children[0] as HTMLElement | undefined;
const row = element.closest('.monaco-tl-row') as HTMLElement | undefined;

const child = element.querySelector('div.monaco-icon-label-container') as Element | undefined;
const childOfChild = child?.querySelector('span.monaco-icon-name-container') as HTMLElement | undefined;
let overflowed = false;
if (childOfChild && child) {
const width = child.clientWidth;
@@ -312,9 +314,8 @@ export class FilesRenderer implements ICompressibleTreeRenderer<ExplorerItem, Fu
const hasDecoration = element.classList.toString().includes('monaco-decoration-iconBadge');
// If it's overflowing or has a decoration show the tooltip
overflowed = overflowed || hasDecoration;
// TODO @lramos15 find a better way to do this, grabs the indent element to position hover above the label
const indentGuideElement = element.parentElement?.parentElement?.children[0] as HTMLElement | undefined;

const indentGuideElement = row?.querySelector('.monaco-tl-indent') as HTMLElement | undefined;
if (!indentGuideElement) {
return;
}
@@ -326,7 +327,10 @@ export class FilesRenderer implements ICompressibleTreeRenderer<ExplorerItem, Fu
additionalClasses: ['explorer-item-hover'],
Copy link
Member

Choose a reason for hiding this comment

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

The icon would be a nice addition and I see you said we can defer that, but I think aligning the position and font exactly with underneath would be really good to go out with this release. It's one of the things that makes the Windows Explorer feature so nice imo.

image

Copy link
Member Author

@lramos15 lramos15 Feb 14, 2023

Choose a reason for hiding this comment

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

image

Hopefully this is close enough. Spent quite some time trying to get the icon to work, but even cloning the row doesn't make it work because it's a psuedo element with a psuedo style. I would have to figure out how to get that pseudo element created in the hover as well.

Copy link
Member

Choose a reason for hiding this comment

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

Looks better 👍

You could maybe align it easy via getComputedStyle and then applying all the relevant font values to the hover element?

skipFadeInAnimation: true,
showPointer: false,
forwardClickEvent: true,
onClick: (e) => {
this.hoverService.hideHover();
element.dispatchEvent(new MouseEvent(e.type, { ...e, bubbles: true }));
},
hoverPosition: HoverPosition.RIGHT,
}, focus) : undefined;
}
@@ -474,7 +478,6 @@ export class FilesRenderer implements ICompressibleTreeRenderer<ExplorerItem, Fu
const realignNestedChildren = stat.nestedParent && themeIsUnhappyWithNesting;

templateData.label.setResource({ resource: stat.resource, name: label }, {
// We use null to indicate an explicit lack of tooltip vs undefined leaves it up to computation later in the rendering
title: isStringArray(label) ? label[0] : label,
fileKind: stat.isRoot ? FileKind.ROOT_FOLDER : stat.isDirectory ? FileKind.FOLDER : FileKind.FILE,
extraClasses: realignNestedChildren ? [...extraClasses, 'align-nest-icon-with-parent-icon'] : extraClasses,
5 changes: 2 additions & 3 deletions src/vs/workbench/services/hover/browser/hover.ts
Original file line number Diff line number Diff line change
@@ -7,7 +7,6 @@ import { createDecorator } from 'vs/platform/instantiation/common/instantiation'
import { IDisposable } from 'vs/base/common/lifecycle';
import { IMarkdownString } from 'vs/base/common/htmlContent';
import { HoverPosition } from 'vs/base/browser/ui/hover/hoverWidget';
import { IContextViewProvider } from 'vs/base/browser/ui/contextview/contextview';

export const IHoverService = createDecorator<IHoverService>('hoverService');

@@ -129,9 +128,9 @@ export interface IHoverOptions {
trapFocus?: boolean;

/**
* Whether to forward all click events to the target element.
* A callback which will be executed when the hover is clicked
*/
forwardClickEvent?: boolean;
onClick?(e: MouseEvent): void;

/*
* The container to pass to {@link IContextViewProvider.showContextView} which renders the hover
13 changes: 2 additions & 11 deletions src/vs/workbench/services/hover/browser/hoverService.ts
Original file line number Diff line number Diff line change
@@ -64,18 +64,9 @@ export class HoverService implements IHoverService {
options.container
);
hover.onRequestLayout(() => provider.layout());
if (options.forwardClickEvent) {
if (options.onClick) {
hoverDisposables.add(addDisposableListener(hover.domNode, EventType.CLICK, e => {
// Forward the click to the target element(s)
if (options.target) {
if ('targetElements' in options.target) {
for (const element of options.target.targetElements) {
element.dispatchEvent(new MouseEvent(e.type, { ...e, bubbles: true }));
}
} else {
options.target.dispatchEvent(new MouseEvent(e.type, { ...e, bubbles: true }));
}
}
options.onClick!(e);
}));
}
if ('targetElements' in options.target) {