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.

Sign up for GitHub

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

Jump to bottom

Scale node labels with the node's size. #1773

Merged
merged 5 commits into from
Aug 10, 2016
Merged

Scale node labels with the node's size. #1773

merged 5 commits into from
Aug 10, 2016

Conversation

foot
Copy link
Contributor

@foot foot commented Aug 9, 2016 •
edited
Loading

  • Within certain bounds. Still have min-label size, mainly effects nodes
    that get really big.
  • Set a max size on nodes too, really big ones lose their border.

Fixes #1692
Fixes #1763

@foot
Scale node labels with the node's size.
5846222 
- Within certain bounds. Still have min-label size, mainly effects nodes
  that get really big.
- Set a max size on nodes too, really big ones lose their border.
@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

Nodes prior to max size restriction

localhost-4042-

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

Max size:

localhost-4042- 1

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

That is for the base zoom level.

Zooming you can decrease/increase still.

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

Hint: chrome devtools device-toolbar is handy for testing window sizes.

@rade
Copy link
Member

rade commented Aug 9, 2016

This does appear to work. I have no idea how. Hopefully @davkal does.

@davkal
Copy link
Contributor

davkal commented Aug 9, 2016

This fix prevents the bad label overlaps, great!

Two visual issues:

  1. select on node, look at label size, deselect, zoom out, select same node, label size is now bigger
  2. when selecting a node, it shortly blows up in size, then the size converges to its end state

@davkal
Copy link
Contributor

davkal commented Aug 9, 2016

Code looks good.
At some point we should implement a proper semantic zoom, that spaces out the nodes a bit more and allows labels to expand.

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

screen shot 2016-08-09 at 18 25 20

Looks kinda fun

@foot
Copy link
Contributor Author

foot commented Aug 9, 2016

Ready for re-review

@rade
Copy link
Member

rade commented Aug 10, 2016

So does this now also fix #1763?

@foot
Copy link
Contributor Author

foot commented Aug 10, 2016

Yep, issue updated.

davkal
davkal reviewed Aug 10, 2016
View reviewed changes
client/app/scripts/charts/node.js
<div className="node-label-wrapper"
style={{pointerEvents: 'all', fontSize, maxWidth: labelWidth}}
style={{pointerEvents: 'all', fontSize, lineHeight: '150%', maxWidth: labelWidth}}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@davkal
Copy link
Contributor

davkal commented Aug 10, 2016

LGTM

@foot
Copy link
Contributor Author

foot commented Aug 10, 2016

This issue fixes label truncation (ellipsis) too, but I don't think thats the same issue as #1760

@rade
Copy link
Member

rade commented Aug 10, 2016

This issue fixes label truncation (ellipsis) too

What exactly is it fixing? Have you got a before/after screenshot?

@foot
Copy link
Contributor Author

foot commented Aug 10, 2016 •
edited
Loading

This branch
screen shot 2016-08-10 at 14 17 37

Master
screen shot 2016-08-10 at 14 17 27

@foot foot merged commit 718bf5b into master Aug 10, 2016
@foot foot deleted the 1692-scale-node-labels-with-nodes branch August 10, 2016 12:32
@rade
Copy link
Member

rade commented Aug 10, 2016

oh ok. I hadn't actually noticed that. What does one have to do to provoke this? I've played with zoom and +/- to no avail.

@foot
Copy link
Contributor Author

foot commented Aug 10, 2016

Nothing in particular just have nice long container names, its noticeable for me on proud-wind.

@rade
Copy link
Member

rade commented Aug 10, 2016

got it. needed to shrink my window vertically to see the problem.

@rade rade mentioned this pull request Aug 16, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@davkal davkal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@foot @rade @davkal