From bf6586017839cfd57674a549ea9279009daf813a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Mon, 13 Aug 2018 21:33:12 -0700 Subject: [PATCH] Change the PagerankTable component cycle (#657) Currently, the PagerankTable creates components in the following pattern: ``` NodeRow (depth=0) > ConnectionRowList (depth=1) > ConnectionRow (depth=2) > ConnectionRowList (depth=3) ``` This commit changes the cycle to the following: ``` NodeRow (depth=0) > ConnectionRowList (depth=0) > ConnectionRow (depth=0) NodeRow (padding=true, depth=1) > ConnectionRowList (depth=1) > ConnectionRow (depth=1) ``` This has some nice properties: First, the context visually resets every time we return to a NodeRow, which makes it feasible to change the score column to always have a context dependent meaning: - for a node row, the score is the score of that node. - for a connection row, the score is the score contribution of that connection - (as of #502): for an aggregation row, the score is the score contribution of that aggregation We think this will be visually clear thanks to the padding around the new NodeRow, along with the new color block indicating a new scope. This design also ensures that every NodeRow has the full width available to it (rather than getting crushed into a progressively smaller area of the table), which will be very convenient for when we add renderers for the nodes. Thanks to @theliamcrawford as the idea for this came during a user study with him. Test plan: The updated unit tests should be comprehensive. Also, try expanding some rows in the cred explorer and verify that the behavior is as described. --- .../credExplorer/pagerankTable/Connection.js | 6 ++-- .../pagerankTable/Connection.test.js | 17 +++++++-- src/app/credExplorer/pagerankTable/Node.js | 24 +++++++++---- .../credExplorer/pagerankTable/Node.test.js | 36 ++++++++++++++----- 4 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/app/credExplorer/pagerankTable/Connection.js b/src/app/credExplorer/pagerankTable/Connection.js index 3a882cd..e40ab90 100644 --- a/src/app/credExplorer/pagerankTable/Connection.js +++ b/src/app/credExplorer/pagerankTable/Connection.js @@ -8,6 +8,7 @@ import type {Connection} from "../../../core/attribution/graphToMarkovChain"; import type {ScoredConnection} from "../../../core/attribution/pagerankNodeDecomposition"; import {DynamicAdapterSet} from "../../adapters/adapterSet"; import {TableRow} from "./TableRow"; +import {NodeRow} from "./Node"; import {edgeVerb, nodeDescription, type SharedProps} from "./shared"; @@ -66,15 +67,16 @@ export class ConnectionRow extends React.PureComponent { ); return ( - diff --git a/src/app/credExplorer/pagerankTable/Connection.test.js b/src/app/credExplorer/pagerankTable/Connection.test.js index f000b42..839fc1d 100644 --- a/src/app/credExplorer/pagerankTable/Connection.test.js +++ b/src/app/credExplorer/pagerankTable/Connection.test.js @@ -8,6 +8,7 @@ import type {Connection} from "../../../core/attribution/graphToMarkovChain"; import {ConnectionRowList, ConnectionRow, ConnectionView} from "./Connection"; import {example} from "./sharedTestUtils"; import {TableRow} from "./TableRow"; +import {NodeRow} from "./Node"; require("../../testUtil").configureEnzyme(); @@ -98,6 +99,14 @@ describe("app/credExplorer/pagerankTable/Connection", () => { const {row, depth} = await setup(); expect(row.props().depth).toBe(depth); }); + it("with indent=1", async () => { + const {row} = await setup(); + expect(row.props().indent).toBe(1); + }); + it("with showPadding=false", async () => { + const {row} = await setup(); + expect(row.props().showPadding).toBe(false); + }); it("with the sourceScore", async () => { const {row, scoredConnection} = await setup(); expect(row.props().score).toBe(scoredConnection.sourceScore); @@ -118,19 +127,23 @@ describe("app/credExplorer/pagerankTable/Connection", () => { expect(cv.props.connection).toEqual(scoredConnection.connection); expect(cv.props.adapters).toEqual(adapters); }); - describe("with a ConnectionRowList as children", () => { + describe("with a NodeRow as children", () => { function getChildren(row) { const children = row.props().children; return shallow(children).instance(); } it("which is a ConnectionRowList", async () => { const {row} = await setup(); - expect(getChildren(row)).toBeInstanceOf(ConnectionRowList); + expect(getChildren(row)).toBeInstanceOf(NodeRow); }); it("which has incremented depth", async () => { const {row, depth} = await setup(); expect(getChildren(row).props.depth).toBe(depth + 1); }); + it("which has padding", async () => { + const {row} = await setup(); + expect(getChildren(row).props.showPadding).toBe(true); + }); it("which has the connection source as its node target", async () => { const {row, scoredConnection} = await setup(); expect(getChildren(row).props.node).toBe(scoredConnection.source); diff --git a/src/app/credExplorer/pagerankTable/Node.js b/src/app/credExplorer/pagerankTable/Node.js index bca5ba2..02391ef 100644 --- a/src/app/credExplorer/pagerankTable/Node.js +++ b/src/app/credExplorer/pagerankTable/Node.js @@ -25,34 +25,46 @@ export class NodeRowList extends React.PureComponent { {sortBy(nodes, (n) => -NullUtil.get(pnd.get(n)).score, (n) => n) .slice(0, maxEntriesPerList) .map((node) => ( - + ))} ); } } -type NodeRowProps = {| +export type NodeRowProps = {| + +depth: number, +node: NodeAddressT, +sharedProps: SharedProps, + +showPadding: boolean, |}; export class NodeRow extends React.PureComponent { render() { - const {node, sharedProps} = this.props; + const {depth, node, sharedProps, showPadding} = this.props; const {pnd, adapters} = sharedProps; const {score} = NullUtil.get(pnd.get(node)); const description = {nodeDescription(node, adapters)}; return ( - + ); } diff --git a/src/app/credExplorer/pagerankTable/Node.test.js b/src/app/credExplorer/pagerankTable/Node.test.js index 1670a09..95d469b 100644 --- a/src/app/credExplorer/pagerankTable/Node.test.js +++ b/src/app/credExplorer/pagerankTable/Node.test.js @@ -11,7 +11,7 @@ import {type NodeAddressT, NodeAddress} from "../../../core/graph"; import {nodeDescription} from "./shared"; import {example} from "./sharedTestUtils"; -import {NodeRowList, NodeRow} from "./Node"; +import {NodeRowList, NodeRow, type NodeRowProps} from "./Node"; require("../../testUtil").configureEnzyme(); @@ -55,6 +55,7 @@ describe("app/credExplorer/pagerankTable/Node", () => { expect(rowNodes.slice().sort()).toEqual(nodes.slice().sort()); rows.forEach((row) => { expect(row.prop("sharedProps")).toEqual(sharedProps); + expect(row.prop("depth")).toEqual(0); }); }); it("creates up to `maxEntriesPerList` `NodeRow`s", async () => { @@ -83,18 +84,34 @@ describe("app/credExplorer/pagerankTable/Node", () => { }); describe("NodeRow", () => { - async function setup() { + async function setup(props: $Shape<{...NodeRowProps}>) { + props = props || {}; const {pnd, adapters, nodes} = await example(); const sharedProps = {adapters, pnd, maxEntriesPerList: 123}; const node = nodes.bar1; - const component = ; - const row = shallow(component).find(TableRow); + const component = shallow( + + ); + const row = component.find(TableRow); return {row, node, sharedProps}; } describe("instantiates a TableRow", () => { it("with the correct depth", async () => { - const {row} = await setup(); - expect(row.props().depth).toBe(0); + const {row: row0} = await setup({depth: 0}); + expect(row0.props().depth).toBe(0); + const {row: row1} = await setup({depth: 1}); + expect(row1.props().depth).toBe(1); + }); + it("with the correct showPadding", async () => { + const {row: row0} = await setup({showPadding: true}); + expect(row0.props().showPadding).toBe(true); + const {row: row1} = await setup({showPadding: false}); + expect(row1.props().showPadding).toBe(false); }); it("with the node's score", async () => { const {row, node, sharedProps} = await setup(); @@ -120,9 +137,10 @@ describe("app/credExplorer/pagerankTable/Node", () => { const {row} = await setup(); expect(getChildren(row)).toBeInstanceOf(ConnectionRowList); }); - it("which has depth=1", async () => { - const {row} = await setup(); - expect(getChildren(row).props.depth).toBe(1); + it("which has the same depth", async () => { + const {row} = await setup({depth: 13}); + const crl = getChildren(row); + expect(crl.props.depth).toBe(13); }); it("which has the node as its node", async () => { const {row, node} = await setup();