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 re_data_ui tooltip to graph view nodes #8311

Merged
merged 6 commits into from
Dec 4, 2024
Merged

Conversation

grtlr
Copy link
Contributor

@grtlr grtlr commented Dec 4, 2024

@grtlr grtlr marked this pull request as ready for review December 4, 2024 13:27
@grtlr grtlr requested a review from abey79 December 4, 2024 13:27
@grtlr grtlr added ui concerns graphical user interface exclude from changelog PRs with this won't show up in CHANGELOG.md labels Dec 4, 2024
Copy link

github-actions bot commented Dec 4, 2024

Web viewer built successfully. If applicable, you should also test it:

  • I have tested the web viewer
Result Commit Link
4c09757 https://rerun.io/viewer/pr/8311

Note: This comment is updated whenever you push a commit.

Copy link
Member

@abey79 abey79 left a comment

Choose a reason for hiding this comment

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

I'm not super convinced by the "show instance for implicit node" part, but having the "regular" tooltip for the "regular" nodes is very nice 👍🏻

crates/viewer/re_space_view_graph/src/ui/draw.rs Outdated Show resolved Hide resolved
Node::Implicit { graph_node, .. } => {
draw_node(ui, center, node.label(), Default::default())
.on_hover_text(format!("Implicit Graph Node: {}", graph_node.as_str(),))
Node::Implicit { edge_instance, .. } => {
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused here. There could be many edge instances which refer to this implicit node right? Is that the first one that you encounter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right—I was too greedy here.

Comment on lines 324 to 333
item_ui::instance_path_button(
ctx,
&query.latest_at_query(),
ctx.recording(),
ui,
Some(query.space_view_id),
&instance_path,
);

instance_path.data_ui_recording(ctx, ui, UiLayout::Tooltip);
Copy link
Member

Choose a reason for hiding this comment

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

Because of my comment above and because this is generally somewhat confusing (and soon broken), I would suggest not having this here, and instead just a text stating "Implicit node created via a reference in the GraphEdge component" or something of that nature.

The edge instance tooltip should be displayed when the actual edge is hovered imo, but that can be left for later (btw, kurbo has point to bezier distance computation so this is something it could help with).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, and I agree. I will change it back!

@grtlr grtlr merged commit 4f1bf04 into main Dec 4, 2024
31 checks passed
@grtlr grtlr deleted the grtlr/graph-tooltips branch December 4, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exclude from changelog PRs with this won't show up in CHANGELOG.md ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add standard tooltip to graph view nodes
2 participants