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: |
gbodeen
force-pushed
the
upstreamable-tooltip-hovers
branch
from
ecf9edd
to
0aea797
Compare
|
New styling examples: Dark Light Solarized Dark I also made a slight change to the Address entries. Now the row matching the visible radix is slightly distinguished. Tomorrow Night Blue theme, Address is hexadecimal: now the Address is octal: |
gbodeen
force-pushed
the
upstreamable-tooltip-hovers
branch
from
0aea797
to
5939ab8
Compare
|
One more small change, the utf8 values are presented in a text color rather than a number color, similar to the Theia memory inspector hovers. |
gbodeen
force-pushed
the
upstreamable-tooltip-hovers
branch
2 times, most recently
from
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.
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.
gbodeen
force-pushed
the
upstreamable-tooltip-hovers
branch
from
6c16770
to
d9e4447
Compare
There was a problem hiding this comment.
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.
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.
How about a dotted underline like this?
There was a problem hiding this comment.
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.
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.
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.
The default hover delay in the VSCode editor is 300ms. I'll use that.
There was a problem hiding this comment.
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! |
gbodeen
force-pushed
the
upstreamable-tooltip-hovers
branch
from
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