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

Grid Mode #1673

Merged
merged 10 commits into from
Aug 3, 2016
Merged

Grid Mode #1673

merged 10 commits into from
Aug 3, 2016

Conversation

foot
Copy link
Contributor

@foot foot commented Jul 13, 2016

Fixes #1621

  • Click row to open details panel
  • Filter table when hitting in search.
  • Add some rank color to the row
  • Remove networks column
  • Add row selection (highlighing)
  • Change enable/disable gridmode icons to something clearer..
  • Add context links to related items
    • Remove hex id cols
  • k8s columns in agg views (check existing cols are sufficient)
  • Add sort column to url state
  • Test in Firefox!!
  • Display label_minor somewhere...
  • Split relatives out into other columns so you can sort by them.
  • Add search text matches
  • The column headings are mis-aligned (most systems other than osx have scrollbars).
  • Fix sorting null/undefined/empty str values.
  • Clarify default sort order onShow
  • Make row selection model clearer
  • Update row selection model, click anywhere on row to select, handle case of highlightling text somehow.
  • Highlight whole row blue

Pushed

@rade
Copy link
Member

rade commented Jul 14, 2016

when did you last rebase this branch? I am having trouble building it.

@foot
Copy link
Contributor Author

foot commented Jul 14, 2016

Yep, origin was in a bit of a bad way until ~2-3 mins ago.

Rebased again onto latest master too just now.

Gonna try building clean too as haven't done that for a bit...

@rade
Copy link
Member

rade commented Jul 16, 2016

re "Add context links to related items".. We need to be careful here not to repeat the mistakes of the details panel - see #921. We could have individual columns here for each of the contexts, e.g. for containers we would have host and pod and image columns. Trouble is that the kind of parents may well differ per item. Hmm. Also, what are we linking to here? Details panels?

Re k8s columns... not sure what you mean by that, but having a grid mode for all the k8s views is not a stretch goal.

Other missing bits:

  • controls - so you don't have to go to the details panel for these
  • highlighting interaction with details panel
    • when I click on a clickable link in a row and the details panel pops up, I expect the row to stay highlighted, just as in the topo mode the node stays highlighted
    • when navigating from the details panel to the grid using the "<-/->" button the row corresponding to the details panel item should be highlighted, just as happens in the topo mode

@rade
Copy link
Member

rade commented Jul 18, 2016

I am not keen on the increased row spacing. It halves the information density compared to the previous version.

@rade
Copy link
Member

rade commented Jul 18, 2016

I am not keen on the increased row spacing

False alarm. Kinda. It's a firefox thing:

screenshot_2016-07-18_15-15-40

@foot
Copy link
Contributor Author

foot commented Jul 18, 2016

Yeah, will play w/ that wrapping, doesn't work that well in chrome w/ narrower browser window.

@rade
Copy link
Member

rade commented Jul 20, 2016

when I use context navigation, e.g. click on an image or host link in a container row, the details panel that pops up is lacking the "</>" button to switch to the table view for that kind of item.

@rade
Copy link
Member

rade commented Jul 20, 2016

There will typically be two "Internet" rows - one for inbound and one for outbound - but since the table doesn't show sub-labels, there is no way to tell which is which.

@rade
Copy link
Member

rade commented Jul 20, 2016

what's the default sort order? Is there one? None of the headings have that little arrow on by default.

@rade
Copy link
Member

rade commented Jul 20, 2016

What is the purpose of the light blue highlighting for the name and context?

Looks like the context is just a list of links. That means I cannot sort by them, e.g. I cannot sort containers by image or host.

The aggregate process and container views (I haven't looked at k8s views yet) are pretty bare. Not much we can do about that until we have aggregate metrics. Though the process-by-name and container-by-dns-name views should at least show process/container counts.

@rade
Copy link
Member

rade commented Jul 20, 2016

I consider the current list of 'stretch' goals to be rather important. Of those, incremental search is the most important - I thought search wasn't working at all because I was so used to seeing the incremental matches from the topo mode. "scroll-to-position" is probably next since the UX can be really puzzling otherwise.

@foot
Copy link
Contributor Author

foot commented Jul 21, 2016

What is the purpose of the light blue highlighting for the name and context?

Its the selector! Allows double-click on other table cells to copy-paste their values w/out opening the details panel. But that not be very clear.

Looks like the context is just a list of links. That means I cannot sort by them, e.g. I cannot sort containers by image or host.

True, that might be nice.

@rade
Copy link
Member

rade commented Jul 22, 2016

What is the purpose of the light blue highlighting for the name and context?

Its the selector! Allows double-click on other table cells to copy-paste their values w/out opening the details panel. But that not be very clear.

This either doesn't work or I don't understand how it's supposed to work.

For me, the blue highlight always shows for the name column when I hover over a row - it never moves to any other cells. And double clicking doesn't do anything special. Well, it pops up the details panel and immediately hides it, and selects the name.

@rade
Copy link
Member

rade commented Jul 22, 2016

A few other niggles...

screenshot_2016-07-22_08-55-00

  • the name heading says 'PROCS' even though I am in the 'CONTAINERS' view
  • the 'CONTAINERS-BY-NAME' heading is truncated and does not pop up a tooltip with the expansion
  • a bunch of columns are truncated, yet there is acres of space in the first column
  • the column headings are mis-aligned - notice how the 'HOSTS' and 'CREATED' headings spill over the column to their right. (somewhat excusable for 'HOSTS' since the column is very narrow, but not for 'CREATED')

I think the freeze-on-hover is too aggressive. Firstly it seems to cover the entire table area, not just the rows, so even having the mouse below the last row still freezes the table. Secondly, it looks like the freeze stops updating completely - I reckon it should only stop rows from moving, not values from updating. Tricky though if the most recent data adds/removes rows. Hmm.

A few alternative strategies to deal with updates while hovering over a row...

  • fix the current row in place and move all the other rows around it to maintain sort order, i.e. effectively "scroll" the table around the current row
  • lay out the table in the correctly sorted order, except for always placing the current row s.t. its position is maintained

The latter is slightly wrong but probably offers a more stable view overall.

@foot
Copy link
Contributor Author

foot commented Jul 25, 2016

I think the freeze-on-hover is too aggressive

Agree

@foot
Copy link
Contributor Author

foot commented Jul 25, 2016

rebased on master.

@rade
Copy link
Member

rade commented Jul 25, 2016

Controls on grid

I'd be happy to see this tackled in a follow-up issue.

@rade
Copy link
Member

rade commented Jul 25, 2016

Consider dancing rows

I'd be happy to see this tackled in a follow-up issue, especially since a) the table only updates every few seconds, and b) until we add controls, nothing bad happens when you accidentally click on the wrong row.

Show-node-in-topo should perhaps scroll table to the position. Consider in conj. w/ dancing rows

ditto.

Most of the other niggles I found have been fixed, except for

a bunch of columns are truncated, yet there is acres of space in the first column

@rade
Copy link
Member

rade commented Jul 26, 2016

There's something weird going on with the sort-order when a field is absent/empty. In the following processes-sorted-by-containers, there are blank entries at the top and near the bottom, though all the non-blank entries are sorted correctly.

screenshot_2016-07-26_08-01-48

@davkal
Copy link
Contributor

davkal commented Jul 26, 2016

Functionality:

  • sort works great, i can see the "top k of X"
    • no pause on hover is fine, user can use pause button
  • mode works seamlessly, switching back and forth between topo works great
  • search works, but it's not clear what's matched, even non-visible fields are matched (maybe display them somehow?)

UX:

  • why put the mode selector next to the node filters? I think it should be superior, like the topologies, or search (long term i'm assuming them to be in a menu bar)
  • why doesnt clicking anywhere on the row select that node? currently i have to click on the label
    • easily clicking on the row seems greatly more important to me than to distinguish clicks on special cells like relatives (if you want to click on relatives you can do it from the details panel). Also, the underlining of relatives is weird (I know it's for consistency, but on the details panel, a click on a relative stacks another panel on top, which does not happen here) If we find that clicking on relatives is a major flow, than we can think about it again
  • why isnt the whole row highlighted blue when selected? seems more in line with the whole node being selected in the graph view
  • How about dynamically adding more columns if the screen gets wide enough? May need introduction of backend column ranking/weight.

Visual:

  • it's hard to follow a row across the screen when column distances get bigger, maybe increase row distance? (also see note about dynamically adding columns)
  • top border feels weird, because there is a shadow suggesting inset, then on the sides no inset can be seen, Escher-Grid?
  • What's the purpose of the bottom border?
  • the blue background over the light selected row background feels weird

@rade
Copy link
Member

rade commented Jul 26, 2016

I see I am not the the only person confused by the blue highlighting :)

maybe increase row distance

Please don't. Information density is key to this mode. Alternating colours, perhaps?

@foot
Copy link
Contributor Author

foot commented Jul 27, 2016

Check this out we get this cool gradient where the alt. colors become more pronounced the further along the row you go because of our bg gradient! Schön oder?

screen shot 2016-07-27 at 19 37 01

@rade
Copy link
Member

rade commented Jul 27, 2016

Sehr schön.

Well, except for that control panel in the bottom left obscuring a whole bunch of entries. But you already know about that, and we can fix that later. And the columns that are infuriatingly truncated because they are just a few pixels too narrow. Please do fix that.

@foot
Copy link
Contributor Author

foot commented Jul 28, 2016

So I'll have a go moving the toggle button and double check those column widths but do you think you might have time to read through some code today @davkal ?

@foot
Copy link
Contributor Author

foot commented Jul 28, 2016

How about dynamically adding more columns if the screen gets wide enough? May need introduction of backend column ranking/weight.

That would be cool.

top border feels weird, because there is a shadow suggesting inset, then on the sides no inset can be seen, Escher-Grid?

I like the effect! the rows are disappearing underneath something. We could turn it off too though. Try it out :).

What's the purpose of the bottom border?

A bordered area for the rows to appear out of. Bottom of the screen and they clash w/ the control icons in bottom right.

@foot
Copy link
Contributor Author

foot commented Jul 28, 2016

I like the effect! the rows are disappearing underneath something.

I've seen this effect done elsewhere but perhaps the shadow only appears when you start scrolling.

@@ -32,7 +32,7 @@ function maybeUpdate(getState) {
receiveNodesDelta(delta);
}
if (deltaBuffer.size > 0) {
updateTimer = setTimeout(maybeUpdate, feedInterval);
updateTimer = setTimeout(() => maybeUpdate(getState), feedInterval);

This comment was marked as abuse.

@davkal
Copy link
Contributor

davkal commented Jul 28, 2016

Commented on JS/CSS code. Minor comments, rest looks good

@foot
Copy link
Contributor Author

foot commented Aug 1, 2016

Continued in

@foot
Copy link
Contributor Author

foot commented Aug 1, 2016

@rade please have a quick look at the columns when you get a chance. The label columns are now as balanced as the other variable columns which should reduce the acreage. Please post screenshot if I'm mis-understanding the issue.

@davkal
Copy link
Contributor

davkal commented Aug 1, 2016

Alternative selector positions:

above search: as the paramount view selector
screen shot 2016-08-01 at 17 26 39

between search and topos (all needs shift to the right): seeing mode and search as 2 dimensions
screen shot 2016-08-01 at 17 29 32

search above: search as overarching element that is affecting all below
screen shot 2016-08-01 at 17 41 17

@rade
Copy link
Member

rade commented Aug 1, 2016

Visually this LGTM now. I suggest filing a separate issue for the selector positioning.

@rade
Copy link
Member

rade commented Aug 1, 2016

Have just noticed that the name doesn't have a tool tip. Which is especially annoying when it is truncated.

@rade
Copy link
Member

rade commented Aug 1, 2016

no longer LGTM. i am sitll running into #1673 (comment).

@rade
Copy link
Member

rade commented Aug 1, 2016

js console shows

vendors.js?4b45a9c…:1 Uncaught Invariant Violation: Minified React error #121; visit http://facebook.github.io/react/docs/error-decoder.html?invariant=121&args[]=93&args[]=78 for the full message or use the non-minified dev environment for full errors and additional helpful warnings.

@foot
Copy link
Contributor Author

foot commented Aug 2, 2016

Cheers, thats a new error message for me! Investigating.

@foot
Copy link
Contributor Author

foot commented Aug 2, 2016

I'm trying to repro, not got it yet. Can you post a report if you get the error consistently.

@rade
Copy link
Member

rade commented Aug 2, 2016

Can you post a report if you get the error consistently.

report.json.gz

@foot
Copy link
Contributor Author

foot commented Aug 2, 2016

Cheers, good catch w/ label tooltip too.

On 2 August 2016 at 10:57, Matthias Radestock notifications@github.com
wrote:

Can you post a report if you get the error consistently.

report.json.gz
https://github.com/weaveworks/scope/files/396192/report.json.gz


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1673 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AABV-OyIhC-D8im2SiaA49gBJJDr_LYxks5qbwZ6gaJpZM4JL0Bu
.

@rade
Copy link
Member

rade commented Aug 2, 2016

process view fix is working for me. and tooltip too.

Other than filing an issue for the selector position (re)location, this LGTM.

foot added 10 commits August 3, 2016 08:37
More graphs!

Rank is not support by dagre any longer..

Quick go at using facebook's fixed-data-table

Kind of working, kind of interesting.

Hack on the details-panel table, supports sorting etc already!

No, this one! Hacks on the details panel's table.

Hovering on the table works! (highlights nodes)

wip get sorting going

Working on sorting, not behaving!

Pulling out methods to fns

Kind of demoable

More hacks to make it demoable
- Change scrolling behaviour to lock headers in place
- Enable filtering (hitting enter in the search bar) in grid-mode
- Little more top-margin for k8s (can have 3 topos) + taller rows.
- Trying out rank-color + node.relatives in the grid-mode
- First pass at selecting rows.
  - Needs a bit more a fiddle, colors + click areas
- Store grid sort direction (asc/desc) in url state
- Simplify node selection to one method. (over-ride existing card)
  - Remove clicking on name directly (links) to overlay new cards for now.
- Playing w/ grid-mode-toggle icons and labels
- Improves rendering in ff, change of shortcut keys for grid-mode-toggle
- Playing w/ clearer selection colors for grid-mode
- Slight change to selection-ui
- Fixes showNodeInTopology button visibility on the details-panel
  - Was using an old heuristic. Table-mode allows you to open child cards
    before the parent.
- Make it clear what the default sort is in tables
  - E.g. always show a sorting caret
- Sort grid-mode columns, first meta then metrics
- dancing-nodes rememdy #1: pause updates onRowHover
- Splits relatives out into their own columns
- Take into account scrollbar width for grid-mode col header position
- Tooltips on table column headers
- grid-mode: fixes first column headers (proc/container/c-by-image)
- Disable pause-on-hover, too aggresive
- reduce label column width a bit (33pc -> 25pc) for big tables
- Filter grid-mode onSearchChange
  - Rather than previous behaviour of waiting for an <enter>
- Show label_minor on pseudo nodes, that might not have much other info
- grid-mode: further reduce width of id column.
- Fixes go tests, properly moves parents into node-summary
- Fixes sorting of string columns w/ missing fields.
  - E.g. uptime. Where -1e-10 > '3days' doesn't work.
- highlight whole row
- don't do anything when selecting text
- Also don't show table borders when no nodes.
- 't' toggles table mode on/off, rather than 't'/v'
- Also adds tooltip for id label column (container/process name)
@foot foot merged commit 5a872da into master Aug 3, 2016
@rade rade deleted the grid-view branch July 5, 2017 13:11
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