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

Add rich tooltips for plugin trees and VSX extensions #10108

Merged
merged 8 commits into from
Oct 14, 2021

Conversation

thegecko
Copy link
Member

@thegecko thegecko commented Sep 15, 2021

Signed-off-by: robmor01 [email protected]

What it does

The plugin-ext support for tree views coming from VSCode plugins doesn't support rendering markdown in tooltips.
This PR introduces functionality to render a rich tooltip when a plugin supplies markdown for that purpose.

It checks that the string passed is markdown before rendering, so shouldn't interfere with normal titles and doesn't instantiate the tooltip renderer until required.

Additionally, this new tooltip functionality has been used to add a tooltip to the VSX extensions panel.

This PR introduces a new direct dependency to react-tooltip (MIT), so let's see what the new License check GitHub CI flow says.

The tooltip has been made available to the entire application and can render html. As this has to be set programmatically, it shouldn't pose a security risk.

Tooltip follows the theming of Theia (see below)

Note: There is a minor addition to the launch.json file for a compound configuration to Launch Browser Backend & Frontend

How to test

Open the VSX extensions panel and observe the tooltips.

To test tooltips on plugin trees, use theia with a plugin which offers known markdown tooltips, e.g. the SVD peripheral viewer of cortex-debug

Screenies

Tooltip support for the VSX-extensions:
Screenshot 2021-10-01 at 19 59 20

Plugin tree tooltip (Dark):
Screenshot 2021-09-15 at 16 25 18

Plugin tree tooltip (Light):
Screenshot 2021-09-15 at 16 25 35

Review checklist

Reminder for reviewers

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@thegecko given the fact we don't have an example where the new tooltip renderer is actually used in the codebase (meaning it is hard to test especially regressions), can we not add a example in the api-samples?

packages/core/src/browser/tooltip-renderer.tsx Outdated Show resolved Hide resolved
packages/core/src/browser/style/tooltip.css Show resolved Hide resolved
@thegecko
Copy link
Member Author

can we not add a example in the api-samples

Depends on the effort involved...

Is api-samples a package or repo?

@vince-fugnitto
Copy link
Member

Is api-samples a package or repo?

It's an extension that is part of our repo and included in the examples (browser and electron) but not published:

The idea is to at least have one example in the repo for test purposes, and given nothing at the moment uses the new tooltip renderer it is hard to test, add features to, and verify for any possible regressions.

@vince-fugnitto vince-fugnitto added the ui/ux issues related to user interface / user experience label Sep 15, 2021
@thegecko
Copy link
Member Author

Still working on an api-example, please bear with me.

I believe all other comments/requests have been resolved.

@vince-fugnitto
Copy link
Member

A good place to use it in the future would be the following like in vscode :)

image

@thegecko thegecko changed the title Add markdown renderer for plugin tree tooltips Add rich tooltips for plugin trees and VSX registry Oct 1, 2021
@thegecko
Copy link
Member Author

thegecko commented Oct 1, 2021

@vince-fugnitto I spent some time trying to devise an api-example for this, but it got too complicated and was taking too long.

I found it far easier to simply implement the vsx-extension tooltips you suggested (see updated PR description).

Would this suffice as a usage example?

@thegecko thegecko changed the title Add rich tooltips for plugin trees and VSX registry Add rich tooltips for plugin trees and VSX extensions Oct 1, 2021
@thegecko
Copy link
Member Author

bump @vince-fugnitto

@vince-fugnitto
Copy link
Member

bump @vince-fugnitto

Sorry @thegecko I didn’t get a chance to review the pull-request yet, I’ll likely do so on Tuesday when I’m back at work. I think implementing it for vsx was a great idea 👍

@thegecko
Copy link
Member Author

I’ll likely do so on Tuesday

No problem!

Comment on lines 191 to 198
},
{
"name": "Launch Browser Backend & Frontend",
"configurations": [
"Launch Browser Backend",
"Attach to Plugin Host",
"Launch Browser Frontend"
]
Copy link
Member

Choose a reason for hiding this comment

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

@thegecko should the changes be added to a separate commit, I believe they are overall unrelated to the pull-request.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

}

get tooltip(): string {
const details = this.getData('readme') || this.description;
Copy link
Member

Choose a reason for hiding this comment

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

@thegecko I believe it might be too much information to use the readme, it will lead to the following issue:

  1. search gitlens
  2. use the tooltip on the first result
vsx-tooltip.mp4

Copy link
Member Author

Choose a reason for hiding this comment

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

Readme removed

@vince-fugnitto vince-fugnitto added the vsx-registry Issues related to Open VSX Registry Integration label Oct 12, 2021
@thegecko
Copy link
Member Author

@vince-fugnitto Comments addressed

}

protected doRender(): React.ReactNode {
return <ReactTooltip id={this.tooltipId} className='theia-tooltip' html={true} delayShow={1000} />;
Copy link
Member

Choose a reason for hiding this comment

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

Could we use some type of TooltipProps where clients can more easily configure how they want their tooltips to behave, and possibly support other react-tooltip options? For example if they want to update delayShow.

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at the props and didn't find much we'd want to expose. I'd also suggest the popup delay is kept consistent across the product

}

protected doRender(): React.ReactNode {
return <ReactTooltip id={this.tooltipId} className='theia-tooltip' html={true} delayShow={1000} />;
Copy link
Member

Choose a reason for hiding this comment

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

The delay in the vsx-registry felt a bit long, any reason you chose 1 second?

Suggested change
return <ReactTooltip id={this.tooltipId} className='theia-tooltip' html={true} delayShow={1000} />;
return <ReactTooltip id={this.tooltipId} className='theia-tooltip' html={true} delayShow={500} />;

Copy link
Member Author

Choose a reason for hiding this comment

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

I timed the vscode popup

Copy link
Member

@msujew msujew Oct 13, 2021

Choose a reason for hiding this comment

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

I agree with Vince here. It seems like the delay is indeed a bit long (depending on which OS you're using). See this setting in the vscode codebase which controlls the delay of the tooltip popup.

I'd be in favor of adding a similar setting to the preferences in here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, I'm on a mac hence the longer delay experienced!

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
I'll give others the opportunity to review as a well!

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Code looks good to me as well! I have one minor comment and also would like to see the hover delay preference added. Afterwards this is good to go 👍

@thegecko
Copy link
Member Author

...would like to see the hover delay preference added

What is wanted here? @vince-fugnitto suggested making it quicker, alternatively it could be editable. Is that globally or on a per popup basis?

@msujew
Copy link
Member

msujew commented Oct 13, 2021

What is wanted here?

@thegecko I left a comment in this thread (I know, github is incredibly bad at showing that someone appended a comment in a review thread). I would like to align with vscode in that regard. The delay would be global in that case.

@thegecko
Copy link
Member Author

@thegecko I left a comment in this thread (I know, github is incredibly bad at showing that someone appended a comment in a review thread). I would like to align with vscode in that regard. The delay would be global in that case.

Ack, I'll do that

@thegecko
Copy link
Member Author

Version and preference suggestions implemented.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@thegecko thegecko merged commit 5d88bf5 into eclipse-theia:master Oct 14, 2021
@vince-fugnitto vince-fugnitto added this to the 1.19.0 milestone Oct 28, 2021
@msujew msujew mentioned this pull request Nov 15, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux issues related to user interface / user experience vsx-registry Issues related to Open VSX Registry Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants