Move WeightConfig into PagerankTable (#798)

Previously, the WeightConfig (and the button that expanded it) were in the credExplorer App. This was a little weird, as there's no reason to play with the weights before you have some Pagerank results to investigate; additionally, it risked confusing new users with a concept that was not yet applicable. Also, the implementation was wonky: the WeightConfig had responsibility for expanding/hiding itself, which gave poor ability to position the button and the WeightConfig separately. Finally, the codepath was untested (vestiges of #604). This commit fixes all three issues: - The WeightConfig and button have moved into PagerankTable - The WeightConfig is now a stateless component, and the parent takes responsibility for deciding when to mount it - Logic for showing/hiding the WeightConfig is now tested.
2018-09-06 18:58:09 -07:00 · 2018-09-06 18:58:09 -07:00 · bf35bbbbda
parent b632bd6188
commit bf35bbbbda
7 changed files with 138 additions and 90 deletions
--- a/src/app/credExplorer/App.js
+++ b/src/app/credExplorer/App.js
@ -9,7 +9,6 @@ import BrowserLocalStore from "../browserLocalStore";

 import {defaultStaticAdapters} from "../adapters/defaultPlugins";
 import {PagerankTable} from "./pagerankTable/Table";
-import {WeightConfig} from "./weights/WeightConfig";
 import {
  type WeightedTypes,
  defaultWeightsForAdapterSet,
@ -86,6 +85,10 @@ export function createApp(
          <PagerankTable
            defaultNodeFilter={GithubPrefix.user}
            adapters={adapters}
+            weightedTypes={this.state.weightedTypes}
+            onWeightedTypesChange={(weightedTypes) =>
+              this.setState({weightedTypes})
+            }
            pnd={pnd}
            maxEntriesPerList={100}
          />
@ -130,11 +133,6 @@ export function createApp(
          >
            Analyze cred
          </button>
-          <WeightConfig
-            onChange={(weightedTypes) => this.setState({weightedTypes})}
-            weightedTypes={this.state.weightedTypes}
-            adapters={this.props.adapters}
-          />
          <LoadingIndicator appState={this.state.appState} />
          {pagerankTable}
        </div>
--- a/src/app/credExplorer/App.test.js
+++ b/src/app/credExplorer/App.test.js
@ -13,7 +13,6 @@ import {defaultWeightsForAdapter} from "./weights/weights";

 import RepositorySelect from "./RepositorySelect";
 import {PagerankTable} from "./pagerankTable/Table";
-import {WeightConfig} from "./weights/WeightConfig";
 import {createApp, LoadingIndicator} from "./App";
 import {uninitializedState} from "./state";
 import {_Prefix as GithubPrefix} from "../../plugins/github/nodes";
@ -126,18 +125,6 @@ describe("app/credExplorer/App", () => {
      });
    }

-    function testWeightConfig(stateFn) {
-      it("creates a working WeightConfig", () => {
-        const {el, setState} = example();
-        setState(stateFn());
-        const wc = el.find(WeightConfig);
-        const wt = defaultWeightsForAdapter(new FactorioStaticAdapter());
-        wc.props().onChange(wt);
-        expect(el.state().weightedTypes).toBe(wt);
-        expect(wc.props().adapters).toBe(el.instance().props.adapters);
-      });
-    }
-
    function testAnalyzeCredButton(stateFn, {disabled}) {
      const adjective = disabled ? "disabled" : "working";
      it(`has a ${adjective} analyze cred button`, () => {
@ -178,8 +165,16 @@ describe("app/credExplorer/App", () => {
          }
          const adapters = state.graphWithAdapters.adapters;
          const pnd = state.pagerankNodeDecomposition;
+          const weightedTypes = el.instance().state.weightedTypes;
          expect(prt.props().adapters).toBe(adapters);
          expect(prt.props().pnd).toBe(pnd);
+          expect(prt.props().weightedTypes).toBe(weightedTypes);
+          const prtWeightedTypesChange = prt.props().onWeightedTypesChange;
+          const newTypes = defaultWeightsForAdapter(
+            new FactorioStaticAdapter()
+          );
+          prtWeightedTypesChange(newTypes);
+          expect(el.instance().state.weightedTypes).toBe(newTypes);
        } else {
          expect(prt).toHaveLength(0);
        }
@ -203,7 +198,6 @@ describe("app/credExplorer/App", () => {
      {analyzeCredDisabled, hasPagerankTable}
    ) {
      describe(suiteName, () => {
-        testWeightConfig(stateFn);
        testRepositorySelect(stateFn);
        testAnalyzeCredButton(stateFn, {disabled: analyzeCredDisabled});
        testPagerankTable(stateFn, hasPagerankTable);
--- a/src/app/credExplorer/pagerankTable/Table.js
+++ b/src/app/credExplorer/pagerankTable/Table.js
@ -9,16 +9,23 @@ import type {PagerankNodeDecomposition} from "../../../core/attribution/pagerank
 import {DynamicAdapterSet} from "../../adapters/adapterSet";
 import type {DynamicPluginAdapter} from "../../adapters/pluginAdapter";
 import {FALLBACK_NAME} from "../../adapters/fallbackAdapter";
+import {type WeightedTypes} from "../weights/weights";
+import {WeightConfig} from "../weights/WeightConfig";

 import {NodeRowList} from "./Node";

 type PagerankTableProps = {|
  +pnd: PagerankNodeDecomposition,
  +adapters: DynamicAdapterSet,
+  +weightedTypes: WeightedTypes,
+  +onWeightedTypesChange: (WeightedTypes) => void,
  +maxEntriesPerList: number,
  +defaultNodeFilter: ?NodeAddressT,
 |};
-type PagerankTableState = {|topLevelFilter: NodeAddressT|};
+type PagerankTableState = {|
+  topLevelFilter: NodeAddressT,
+  showWeightConfig: boolean,
+|};
 export class PagerankTable extends React.PureComponent<
  PagerankTableProps,
  PagerankTableState
@ -38,13 +45,42 @@ export class PagerankTable extends React.PureComponent<
      props.defaultNodeFilter,
      NodeAddress.empty
    );
-    this.state = {topLevelFilter};
+    this.state = {topLevelFilter, showWeightConfig: false};
+  }
+
+  renderConfigurationRow() {
+    const {showWeightConfig} = this.state;
+    return (
+      <div style={{display: "flex"}}>
+        {this.renderFilterSelect()}
+        <span style={{flexGrow: 1}} />
+        <button
+          onClick={() => {
+            this.setState(({showWeightConfig}) => ({
+              showWeightConfig: !showWeightConfig,
+            }));
+          }}
+        >
+          {showWeightConfig
+            ? "Hide weight configuration"
+            : "Show weight configuration"}
+        </button>
+      </div>
+    );
  }

  render() {
+    const {showWeightConfig} = this.state;
    return (
      <div style={{marginTop: 10}}>
-        {this.renderFilterSelect()}
+        {this.renderConfigurationRow()}
+        {showWeightConfig && (
+          <WeightConfig
+            adapters={this.props.adapters.static()}
+            weightedTypes={this.props.weightedTypes}
+            onChange={(wt) => this.props.onWeightedTypesChange(wt)}
+          />
+        )}
        {this.renderTable()}
      </div>
    );
--- a/src/app/credExplorer/pagerankTable/Table.test.js
+++ b/src/app/credExplorer/pagerankTable/Table.test.js
@ -7,42 +7,87 @@ import {NodeAddress, type NodeAddressT} from "../../../core/graph";

 import {PagerankTable} from "./Table";
 import {example, COLUMNS} from "./sharedTestUtils";
+import {NodeRowList} from "./Node";
+import {WeightConfig} from "../weights/WeightConfig";
+import {defaultWeightsForAdapter} from "../weights/weights";
+import {FactorioStaticAdapter} from "../../adapters/demoAdapters";

 require("../../testUtil").configureEnzyme();
 describe("app/credExplorer/pagerankTable/Table", () => {
  describe("PagerankTable", () => {
-    it("renders thead column order properly", async () => {
-      const {pnd, adapters} = await example();
+    async function setup(defaultNodeFilter?: NodeAddressT) {
+      const {pnd, adapters, weightedTypes} = await example();
+      const onWeightedTypesChange = jest.fn();
+      const maxEntriesPerList = 321;
      const element = shallow(
        <PagerankTable
-          defaultNodeFilter={null}
+          defaultNodeFilter={defaultNodeFilter}
+          weightedTypes={weightedTypes}
+          onWeightedTypesChange={onWeightedTypesChange}
          pnd={pnd}
          adapters={adapters}
-          maxEntriesPerList={1}
+          maxEntriesPerList={maxEntriesPerList}
        />
      );
+      return {pnd, adapters, element, maxEntriesPerList, onWeightedTypesChange};
+    }
+    it("renders thead column order properly", async () => {
+      const {element} = await setup();
      const th = element.find("thead th");
      const columnNames = th.map((t) => t.text());
      expect(columnNames).toEqual(COLUMNS());
    });

-    describe("has a filter select", () => {
-      async function setup(defaultNodeFilter?: NodeAddressT) {
-        const {pnd, adapters} = await example();
-        const element = shallow(
-          <PagerankTable
-            defaultNodeFilter={defaultNodeFilter}
-            pnd={pnd}
-            adapters={adapters}
-            maxEntriesPerList={1}
-          />
-        );
-        const label = element.find("label");
-        const options = label.find("option");
-        return {pnd, adapters, element, label, options};
+    describe("has a WeightConfig", () => {
+      function findButton(element) {
+        const button = element
+          .findWhere(
+            (x) =>
+              x.text() === "Show weight configuration" ||
+              x.text() === "Hide weight configuration"
+          )
+          .find("button");
+        expect(button).toHaveLength(1);
+        return button;
      }
+      it("which is not present by default", async () => {
+        const {element} = await setup();
+        expect(element.find(WeightConfig)).toHaveLength(0);
+        const button = findButton(element);
+        expect(button.text()).toEqual("Show weight configuration");
+      });
+      it("which is present when the WeightConfig button is pushed", async () => {
+        const {element, onWeightedTypesChange} = await setup();
+        let button = findButton(element);
+        expect(button.text()).toEqual("Show weight configuration");
+        button.simulate("click");
+        button = findButton(element);
+        expect(button.text()).toEqual("Hide weight configuration");
+        expect(button).toHaveLength(1); // Its text changed
+        const wc = element.find(WeightConfig);
+        expect(wc).toHaveLength(1);
+        expect(wc.props().weightedTypes).toBe(
+          element.instance().props.weightedTypes
+        );
+        const wt = defaultWeightsForAdapter(new FactorioStaticAdapter());
+        wc.props().onChange(wt);
+        expect(onWeightedTypesChange).toHaveBeenCalledWith(wt);
+        expect(onWeightedTypesChange).toHaveBeenCalledTimes(1);
+      });
+      it("which is hidden when the WeightConfig button is pushed twice", async () => {
+        const {element} = await setup();
+        findButton(element).simulate("click");
+        findButton(element).simulate("click");
+        let button = findButton(element);
+        expect(button.text()).toEqual("Show weight configuration");
+        expect(element.find(WeightConfig)).toHaveLength(0);
+      });
+    });
+
+    describe("has a filter select", () => {
      it("with expected label text", async () => {
-        const {label} = await setup();
+        const {element} = await setup();
+        const label = element.find("label");
        const filterText = label
          .find("span")
          .first()
@ -50,7 +95,9 @@ describe("app/credExplorer/pagerankTable/Table", () => {
        expect(filterText).toMatchSnapshot();
      });
      it("with expected option groups", async () => {
-        const {options} = await setup();
+        const {element} = await setup();
+        const label = element.find("label");
+        const options = label.find("option");
        const optionsJSON = options.map((o) => ({
          valueString: NodeAddress.toString(o.prop("value")),
          style: o.prop("style"),
@ -59,7 +106,9 @@ describe("app/credExplorer/pagerankTable/Table", () => {
        expect(optionsJSON).toMatchSnapshot();
      });
      it("with the ability to filter nodes passed to NodeRowList", async () => {
-        const {element, options} = await setup();
+        const {element} = await setup();
+        const label = element.find("label");
+        const options = label.find("option");
        const option = options.at(2);

        const value = option.prop("value");
@ -93,29 +142,17 @@ describe("app/credExplorer/pagerankTable/Table", () => {
    });

    describe("creates a NodeRowList", () => {
-      async function setup() {
-        const {adapters, pnd} = await example();
-        const maxEntriesPerList = 1;
-        const element = shallow(
-          <PagerankTable
-            defaultNodeFilter={null}
-            pnd={pnd}
-            adapters={adapters}
-            maxEntriesPerList={maxEntriesPerList}
-          />
-        );
-        const nrl = element.find("NodeRowList");
-        return {adapters, pnd, element, nrl, maxEntriesPerList};
-      }
      it("with the correct SharedProps", async () => {
-        const {nrl, adapters, pnd, maxEntriesPerList} = await setup();
+        const {element, adapters, pnd, maxEntriesPerList} = await setup();
+        const nrl = element.find(NodeRowList);
        const expectedSharedProps = {adapters, pnd, maxEntriesPerList};
-        expect(nrl.prop("sharedProps")).toEqual(expectedSharedProps);
+        expect(nrl.props().sharedProps).toEqual(expectedSharedProps);
      });
      it("including all nodes by default", async () => {
-        const {nrl, pnd} = await setup();
+        const {element, pnd} = await setup();
+        const nrl = element.find(NodeRowList);
        const expectedNodes = Array.from(pnd.keys());
-        expect(nrl.prop("nodes")).toEqual(expectedNodes);
+        expect(nrl.props().nodes).toEqual(expectedNodes);
      });
    });
  });
--- a/src/app/credExplorer/pagerankTable/sharedTestUtils.js
+++ b/src/app/credExplorer/pagerankTable/sharedTestUtils.js
@ -2,16 +2,18 @@

 import {dynamicAdapterSet} from "../../adapters/demoAdapters";
 import {pagerank} from "../../../core/attribution/pagerank";
+import {defaultWeightsForAdapterSet} from "../weights/weights";

 export const COLUMNS = () => ["Description", "", "Cred"];

 export async function example() {
  const adapters = await dynamicAdapterSet();
+  const weightedTypes = defaultWeightsForAdapterSet(adapters.static());
  const graph = adapters.graph();
  const pnd = await pagerank(graph, (_unused_Edge) => ({
    toWeight: 1,
    froWeight: 1,
  }));

-  return {adapters, pnd};
+  return {adapters, pnd, weightedTypes};
 }
--- a/src/app/credExplorer/weights/WeightConfig.js
+++ b/src/app/credExplorer/weights/WeightConfig.js
@ -15,40 +15,23 @@ type Props = {|
  +onChange: (WeightedTypes) => void,
 |};

-type State = {|
-  expanded: boolean,
-|};
-
-export class WeightConfig extends React.Component<Props, State> {
+export class WeightConfig extends React.Component<Props> {
  constructor(props: Props): void {
    super(props);
-    this.state = {
-      expanded: false,
-    };
  }

  render() {
-    const {expanded} = this.state;
    return (
      <React.Fragment>
-        <button
-          onClick={() => {
-            this.setState(({expanded}) => ({expanded: !expanded}));
+        <div
+          style={{
+            display: "flex",
+            flexWrap: "wrap",
+            justifyContent: "space-between",
          }}
        >
-          {expanded ? "Hide weight configuration" : "Show weight configuration"}
-        </button>
-        {expanded && (
-          <div
-            style={{
-              display: "flex",
-              flexWrap: "wrap",
-              justifyContent: "space-between",
-            }}
-          >
-            {this._renderPluginWeightConfigs()}
-          </div>
-        )}
+          {this._renderPluginWeightConfigs()}
+        </div>
      </React.Fragment>
    );
  }
--- a/src/app/credExplorer/weights/WeightConfig.test.js
+++ b/src/app/credExplorer/weights/WeightConfig.test.js
@ -31,8 +31,6 @@ describe("app/credExplorer/weights/WeightConfig", () => {
          onChange={onChange}
        />
      );
-      // TODO(@decentralion): WeightConfig stops expanding itself
-      el.setState({expanded: true});
      return {el, adapters, types, onChange};
    }
    it("creates a PluginWeightConfig for every non-fallback adapter", () => {