-
Notifications
You must be signed in to change notification settings - Fork 10
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
Hovers using PrimeReact Tooltip #87
Hovers using PrimeReact Tooltip #87
Conversation
Thanks a lot, @gbodeen , adding those tooltips back is really useful! |
@jreineckearm , I can take a look through the styles Theia uses. Let me know if there are any divergences from Theia's version that you'd prefer. |
Thanks @gbodeen ! I think the styling in the Theia version was good. In general, I think it would be good to be close to what other built-in views have. Thinking here in particular about the green variable value color in the Variables window. Or the green and blue tones used in some variable hovers in the source editor while debugging. But I can see that the styling in the Theia version is already pretty much that: |
ecf9edd
to
0aea797
Compare
0aea797
to
5939ab8
Compare
f97f147
to
039cc4a
Compare
Looks good, @gbodeen ! I like the table approach in the tooltip! Was thinking about the used format name color. As orange could indicate an error or a warning. But would suggest to keep it because it's used in various places in VS Code debug. And we have only a limited set of colors that can be derived from VS Code. I'll add a suggestion to the tooltip creation for
Both are debatable. So, no super-strong opinion on adding them if you prefer to go without. |
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.
Hi @gbodeen! Philip is out this week so he asked me to have a look at the PR. It really looks nice and seems to work very well, so thank you very much for that great contribution!
I do have a few comments but nothing major that should prevent this PR from being merged. I just have the feeling that the tooltips are a bit "aggressive" as they appear immediately, maybe a short delay could help. But this is personal taste and if we feel like this might irritate users we can always create a preference for it afterwards as well.
6c16770
to
d9e4447
Compare
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.
Hi @gbodeen, thank you very much for the update! I think we are almost there. I do have some minor comments so I'd be interested to get your opinion on that. Otherwise, I think this is in a good state. Great work!
Btw, could you please add a separate commit on top next time so it is more obvious which parts have changed? That would make the re-reviewing process a bit easier, thank you!
src/webview/columns/data-column.tsx
Outdated
@@ -47,11 +47,11 @@ export class DataColumn implements ColumnContribution { | |||
const isLast = i + 1n >= range.endAddress; | |||
const style: React.CSSProperties | undefined = isLast ? undefined : this.byteGroupStyle; | |||
|
|||
groups.push(<span className='byte-group' style={style} key={i.toString(16)}>{words}</span>); | |||
groups.push(<span className='byte-group hoverable' data-column='data' style={style} key={i.toString(16)}>{words}</span>); |
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.
Do you think it might make sense to apply additional hover feedback/highlighting on the span? Since we might have several words per group it could really make it more obvious which part we actually show hover information for.
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 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.
It is very subtle but I like it, thank you!
target=".hoverable" | ||
onBeforeShow={this.handleOnBeforeTooltipShow} | ||
onHide={this.handleOnTooltipHide} | ||
showDelay={1000} |
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.
I know I requested the delay but 1000 feels like too long now, maybe going down a bit might help? What do you think, @jreineckearm?
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.
I am fine going down a little with that. We should only make sure that the user has enough time without the tooltip when moving through the table in a stop and go manner, for example when using the mouse as a kind of means to guide their eye movement (not sure if that made sense).
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.
The default hover delay in the VSCode editor is 300ms. I'll use that.
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.
Change looks good to me now! Thank you very much @gbodeen!
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.
Thank you very much @gbodeen! This looks really good, functionality- and code-wise.
Also thanks @martin-fleck-at and @jreineckearm for your reviews and inputs! Now my job is pretty simple :-)
I've seen there are some conflicts. Can you please resolve those conflicts and I'm happy to merge, or @colin-grant-work can merge this after rebasing. Thank you! |
9d386a4
to
a01d06e
Compare
What it does
Handles issue 66 by way of recreating the hovers from the Theia memory hover renderer. PrimeReact has a Tooltip component that automatically handles visibility and placement, though in my testing it sometimes gets a little behind the current state if you rapidly switch hovering between many fields.
Variable in the Variables column:
Group in the Data column:
Address:
How to test
Hover over entries in either the address, data, or variable column. It should continue to work even after changing display options.
Review checklist
Reminder for reviewers