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

UI: Add tooltips to the visualization wrapper #6006

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

maxlang
Copy link
Contributor

@maxlang maxlang commented Apr 12, 2016

This change is Reviewable

@maxlang
Copy link
Contributor Author

maxlang commented Apr 12, 2016

cc @mrtracy Ready for review, but working on the actual tooltip text with @jseldess

@maxlang maxlang force-pushed the maxlang/ui-tooltips branch 2 times, most recently from 49e8093 to 722a3a8 Compare April 13, 2016 18:22
@maxlang
Copy link
Contributor Author

maxlang commented Apr 13, 2016

Currently only includes 3 tooltips, and doesn't include a separate warning state. The info icon only appears if the visualization has a tooltip, but the warning icon always appears in the error/warning state.

screen shot 2016-04-13 at 2 05 32 pm

screen shot 2016-04-13 at 2 05 37 pm

screen shot 2016-04-13 at 2 05 56 pm

@maxlang maxlang changed the title DO NOT MERGE Add tooltips to the visualization wrapper UI: Add tooltips to the visualization wrapper Apr 13, 2016
@mrtracy
Copy link
Contributor

mrtracy commented Apr 13, 2016

Reviewed 5 of 9 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


ui/styl/modules/palette.styl, line 62 [r2] (raw file):
Any rease


ui/styl/partials/layout.styl, line 579 [r2] (raw file):
I don't think you're using .left anywhere.


ui/styl/partials/layout.styl, line 610 [r2] (raw file):
What's the point of the .left/.right classes if you're applying values based on hierarchy like this?


Comments from Reviewable

@mrtracy
Copy link
Contributor

mrtracy commented Apr 13, 2016

:lgtm:, suggestions provided on how to clean up the CSS a bit.

I would like for the tooltips to eventually position themselves more intelligently (for example, based on mouse cursor position); explicitly specifying '.left/.right' or doing it based on hierarchy is fine for now, though (although I would suggest picking one approach or the other, if possible).


Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@maxlang maxlang force-pushed the maxlang/ui-tooltips branch from 722a3a8 to 20cf214 Compare April 13, 2016 19:50
@maxlang
Copy link
Contributor Author

maxlang commented Apr 13, 2016

Cleaned up the CSS to just use the hierarchy as suggested. I'll hold off on doing something more intelligent until I see if there's a react component that can handle the various issues involved with this instead (eg detecting if a tooltip would go off the page and repositioning it.)


Review status: 7 of 9 files reviewed at latest revision, 3 unresolved discussions.


ui/styl/modules/palette.styl, line 62 [r2] (raw file):
?


ui/styl/partials/layout.styl, line 579 [r2] (raw file):
Removed


ui/styl/partials/layout.styl, line 610 [r2] (raw file):
Removed


Comments from Reviewable

@maxlang maxlang force-pushed the maxlang/ui-tooltips branch 3 times, most recently from 90cdd9e to 1b45fbb Compare April 13, 2016 21:20
@jseldess
Copy link
Contributor

Review status: 7 of 9 files reviewed at latest revision, 11 unresolved discussions.


ui/ts/pages/cluster.ts, line 100 [r4] (raw file):
I'd go with "The total number of nodes in the cluster."


ui/ts/pages/cluster.ts, line 103 [r4] (raw file):
Can we make this tootltip dynamic? If so, I'd suggest:

"You are using ~3GB of ~80GB storage capacity across all nodes."

If it needs to be static, we could try:

"The estimated percentage of total storage capacity currently in use."


ui/ts/pages/cluster.ts, line 129 [r4] (raw file):
For Query Time, we could try this:

"The estimated time between query request and response, in milliseconds."

Not sure how to explain the percentiles here. Need to work with someone on that.


ui/ts/pages/cluster.ts, line 142 [r4] (raw file):
Maybe: "The percentage of CPU used by CockroachDB (User %) and system-level operations (Sys %) across all nodes."


ui/ts/pages/cluster.ts, line 151 [r4] (raw file):
I'd suggest: "The average memory in use across all nodes."


ui/ts/pages/cluster.ts, line 158 [r4] (raw file):
Suggested tooltip: "The total number of active SQL connections to the cluster."


ui/ts/pages/cluster.ts, line 170 [r4] (raw file):
Suggested tooltip: "The amount on network traffic sent to and from the SQL system, in bytes."


ui/ts/pages/cluster.ts, line 178 [r4] (raw file):
Suggested tooltip: "The number of SELECT statements, averaged over a 10 second period."


Comments from Reviewable

@jseldess
Copy link
Contributor

ui/ts/pages/cluster.ts, line 170 [r4] (raw file):
Spelling mistake in there. Meant: "The amount of network traffic sent to and from the SQL system, in bytes."


Comments from Reviewable

@maxlang
Copy link
Contributor Author

maxlang commented Apr 14, 2016

@jseldess Here are all the current tooltips.

screen shot 2016-04-14 at 1 27 48 pm

screen shot 2016-04-14 at 1 27 42 pm

screen shot 2016-04-14 at 1 27 36 pm

screen shot 2016-04-14 at 1 27 29 pm

screen shot 2016-04-14 at 1 27 22 pm

screen shot 2016-04-14 at 1 27 17 pm

screen shot 2016-04-14 at 1 27 11 pm

screen shot 2016-04-14 at 1 27 06 pm

screen shot 2016-04-14 at 1 26 57 pm

@jseldess
Copy link
Contributor

LGTM, @maxlang. Should we add tooltips for other tabs in this PR, or would you rather do that in a separate PR?

@maxlang maxlang force-pushed the maxlang/ui-tooltips branch from 1b45fbb to 61be7da Compare April 18, 2016 16:21
@maxlang
Copy link
Contributor Author

maxlang commented Apr 18, 2016

@jseldess yeah let's do it in a separate PR. Let's see if we can go through the rest of the graphs with @cuongdo and @mrtracy

@maxlang maxlang merged commit e134e24 into cockroachdb:master Apr 18, 2016
@maxlang maxlang deleted the maxlang/ui-tooltips branch April 18, 2016 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants