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

Fix selection highlighting in blueprint tree view #2824

Merged
merged 2 commits into from
Jul 26, 2023

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Jul 26, 2023

What

Before:

image

After:

image

This PR also reverts #2796 (fixes #2828).

Changes

Fixes #2728

Checklist

@abey79 abey79 added 🪳 bug Something isn't working ui concerns graphical user interface labels Jul 26, 2023
@abey79 abey79 requested a review from martenbjork July 26, 2023 08:04
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

great to have that fixed, was visually very annoying before!

Please check on the leaking ui.visuals_mut() changes before merging

@@ -459,13 +483,19 @@ fn blueprint_row_with_buttons(

// The main button might have been highlighted because what it was referring
// to was hovered somewhere else, and then we also want it highlighted here.
if button_hovered || main_button_response.highlighted() {
if button_hovered || main_button_response.highlighted() || selected {
Copy link
Member

Choose a reason for hiding this comment

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

is checking for main_button_response.highlighted() even necessary now that selected is passed in? I'd expect that any time main_button_response.highlighted() is true, selected is true as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Things can be highlighted without being selected by virtue of the corresponding entities being hovered over in the timeline/stream tree, so this check is indeed necessary.

crates/re_viewport/src/viewport_blueprint_ui.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@martenbjork martenbjork left a comment

Choose a reason for hiding this comment

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

This is great! Small change, but it makes the UI feel much more solid and predictable. 💯🚢

@abey79
Copy link
Member Author

abey79 commented Jul 26, 2023

@martenbjork there is aspect to this PR that might be improved, it's the hover state of the "minus" and "eye" buttons when the item is selected:

image (mouse pointer is missing from the screenshot, it was hovering over the `–` button)

Hover feedback here would be nice, but I'm unsure how to best implement it. Can you propose something? It'd be ideal to create a new issue as we would like to merge this as is (it's already much better than before) and ship in a 0.8 alpha release due very soon.

@abey79 abey79 added this to the 0.8 milestone Jul 26, 2023
@martenbjork
Copy link
Contributor

I agree, that hover state needs some love. But I agree, let's ship this and follow up with the details. 🚢

I'm currently polishing all these details so we'll have a better design soon!

@abey79 abey79 changed the title Fix selection (and sizing) in blueprint tree view Fix selection highlighting in blueprint tree view Jul 26, 2023
@Wumpf Wumpf merged commit c3487e2 into main Jul 26, 2023
@Wumpf Wumpf deleted the antoine/blueprint_tree_selection_2728 branch July 26, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revert #2796 for the 0.8 release Fix "selected" state when hovering a blueprint item
3 participants