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

Make space view names optional and subdue placeholder view label in the UI #4682

Merged
merged 17 commits into from
Jan 17, 2024

Conversation

abey79
Copy link
Member

@abey79 abey79 commented Jan 4, 2024

What

image image image image

TODO (future PR)

The selection history popup still needs much work (#4678):

image

Checklist

  • I have read and agree to Contributor Guide and the Code of Conduct
  • I've included a screenshot or gif (if applicable)
  • I have tested the web demo (if applicable):
  • The PR title and labels are set such as to maximize their usefulness for the next release's CHANGELOG

abey79 added 2 commits January 4, 2024 14:49
TODO:
- better default name heuristics
- fix formatting everywhere in the UI
@abey79 abey79 changed the title Antoine/spaceview optional name Make space view names optional and subdue placeholder view label in the UI Jan 4, 2024
@abey79 abey79 added ui concerns graphical user interface include in changelog labels Jan 4, 2024
@abey79 abey79 marked this pull request as ready for review January 5, 2024 13:11
crates/re_viewport/src/viewport_blueprint_ui.rs Outdated Show resolved Hide resolved
Comment on lines 945 to 946
// TODO(ab): use design tokens
text_color = text_color.gamma_multiply(0.5);
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 not sure we should both be doing italics and transparency. Transparency also has an opportunity to be confused with a "grayed out" disabled look. What says @martenbjork ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Italics only adds to the visual clutter without conveying the "unnamed" nature of the space view. We should decide:

  1. either we make the unnamed nature explicit (the the visual language should be strong)
  2. or we don't (then we apply no formatting at all)

I'm in favour of (1) but happy to discuss how exactly this should be formatted.

For record, we had explored an alternative way of conveying the unnamed nature via the placeholder name (e.g. "unnamed view of XXX"), but ruled it out for its visual clutter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Punting on this. With #4802, I will further isolate the location where this formatting happens, so should be easy to revisit.

Comment on lines 328 to 329
ListItem::new(ctx.re_ui, label)
.unnamed_style(!named)
Copy link
Member

Choose a reason for hiding this comment

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

ListItem::new already takes an WidgetText argument. Doesn't it make more sense to separate the formatting of the label from the rest of the ListItem logicl?

Copy link
Member Author

@abey79 abey79 Jan 15, 2024

Choose a reason for hiding this comment

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

Excellent point. All this styling stuff cramed into ListWidget was really annoying me. I'll do that in a separate PR though, as this cleanup should go beyond what's touched by this PR:

crates/re_viewer/src/ui/selection_panel.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/space_view.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/space_view.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/space_view.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/space_view.rs Outdated Show resolved Hide resolved
crates/re_viewport/src/viewport_blueprint_ui.rs Outdated Show resolved Hide resolved
@teh-cmc
Copy link
Member

teh-cmc commented Jan 10, 2024

Drafting this until @abey79 is back so it gets out of the review queue.

@teh-cmc teh-cmc marked this pull request as draft January 10, 2024 07:36
@abey79 abey79 marked this pull request as ready for review January 15, 2024 10:22
@abey79 abey79 merged commit 57263d1 into main Jan 17, 2024
40 checks passed
@abey79 abey79 deleted the antoine/spaceview-optional-name branch January 17, 2024 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include in changelog ui concerns graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make space view display_name optional
3 participants