Refactor UI spec "order" property #79

emlys · 2021-01-12T01:46:03Z

As discussed in #31, this PR refactors the UI spec "order" property into its own top-level attribute.

As it is now, the UI spec is no longer optional. Allowing it to stay optional would complicate this PR slightly so let's decide if that's a feature we still want.
I have only edited one UI spec file (carbon) to fit the proposed new format. Once the change of format gets approved I'll update the other spec files.

emlys · 2021-01-12T01:47:06Z

src/components/SetupTab/index.jsx

    this.state = {
      argsValues: null,
-      argsValidation: {},
-      argsValid: false,
-      sortedArgKeys: null,


since this is now just this.props.uiSpec.order I don't think it needs to be part of the state anymore

davemfish

This looks like a great start @emlys . It looks like the code & the spec files will all be getting simpler and easier to read, so that's great!

I had one minor comment on the spec itself. And then my other suggestion is to get the tests passing and then we can do another closer review of the changes from there. There are several tests that define UI specs and test the various functionalities. Ideally, those tests can be made passing just by updating their spec data.

davemfish · 2021-01-12T17:38:07Z

src/ui_data/natcap.invest.carbon.json

+        "lulc_cur_path": {},
+        "calc_sequestration": {
+            "ui_option": "disable",
+            "ui_control": [


Even though I came up with them, these property names kinda trip me up, like I always have to pause and think about what they really mean.

ui_control might be better as control_targets because it holds an array of the args that are under control of calc_sequestration. And ui_option again is the option applied to the targets, not to the controller, so maybe better as control_option? Those are just suggestions, feel free to go with whatever makes sense to you.

control_targets and control_option sound good to me!

emlys · 2021-01-12T20:51:39Z

After working on this more, I'm thinking that the UI spec files should be required, but each arg in the spec file should be optional. Since the order property is moved to the top-level, many args will have no properties. So this should be allowed (no argsOptions.a):

{
    order: [['a', 'b'], ['c']],
    argsOptions: {
        b: {control_targets: ['c']},
        c: {control_option: 'disable'}
    }
}

davemfish

I really like this redesign! I left some miscellaneous comments inline to check out. And we should look into the failing puppeteer test. I can try to reproduce that on Windows if it's not easy for you to reproduce and debug.

And one more thought I had is about some sort of testing/validation for the json files since they are now required and things might break in unexpected ways if one is missing or malformed. Do you have any thoughts about this? I could imagine a CI step that just validates their structure without running any part of the React app. Or way at the other end of the spectrum, we could have puppeteer click on every model to see that it loads. Whatever we do I think it's okay to push it to a separate issue, maybe flagged with the beta release milestone.

davemfish · 2021-01-14T20:07:09Z

src/components/SetupTab/index.jsx

-    // invest model (e.g. Rec model's 'port' & 'hostname' args).
-    if (argkey === 'n_workers'
-      || argsSpec[argkey].order === 'hidden') { return; }
+  // Object.keys(argsSpec).forEach((argkey) => {


A leftover commented line here.

davemfish · 2021-01-14T20:30:06Z

src/components/SetupTab/index.jsx

@@ -83,18 +83,22 @@ function initializeArgValues(argsSpec, argsDict) {
      touched: !initIsEmpty, // touch them only if initializing with values
    };
  });
-  return ({ argsValues: argsValues, argsValidation: argsValidation });
+  return ({ argsValues: argsValues });


Good call moving this argsValidation initialization stuff out of this function and into the constructor.

Since we're only returning one object now, I think we can drop the nested structure here and just return argsValues itself.

davemfish · 2021-01-14T20:31:07Z

src/components/SetupTab/index.jsx

-      // these argkeys do not get rendered inputs
-      if (argkey === 'n_workers'
-        || argsSpec[argkey].order === 'hidden') { return; }
+    let { argsValues } = initializeArgValues(argsSpec, uiSpec, argsInitValues || {});


Related to the comment about this functions return, we probably won't need to "destructure" the returned value.

davemfish · 2021-01-14T20:39:33Z

src/components/SetupTab/index.jsx

-      // Update any dependent args in response to this arg's value
+    // Update any dependent args in response to each arg's value
+    // uiSpec.order is a 2D array so flatten it before iterating
+    uiSpec.order.flat().forEach((argkey) => {
      const argumentSpec = { ...argsSpec[argkey] };


I think this line goes with all the stuff that was removed here, so it should go as well. It was only there to make all the subsequent references to argsSpec[argkey] properties a little less verbose.

davemfish · 2021-01-14T20:41:27Z

src/components/SetupTab/index.jsx

-          sortedGroup.map((item) => Object.values(item)[0])
-        );
-      }
-    });


Not sad to see this whole mess go! And pretty good evidence that the new design is the better design.

davemfish · 2021-01-14T20:53:00Z

src/components/SetupTab/index.jsx

      const updatedArgsValues = toggleDependentInputs(
-        this.props.argsSpec, argsValues, key
+        this.props.uiSpec, argsValues, key


Not mandatory, but here's a case where we could use destructuring to make things a little less verbose. const { uiSpec } = this.props; I think eslint might suggest this, and I tend to do it in places where there are many this.props.... references.

davemfish · 2021-01-14T21:11:55Z

src/ui_data/README.md

@@ -0,0 +1,21 @@
+# Explanation of UI spec JSON format


Nice addition!

davemfish · 2021-01-14T21:12:53Z

src/ui_data/README.md

+
+        * `"control_option"`: A string that determines the CSS style to apply to this arg's input field, conditional
+            on the state of the controlling field. If `"control_option": "x"`, then the style `arg-x` is applied.
+            Currently `"disable"`, `"hide"`, and `"group"` are available.


I don't think "group" is an option, is it?

There is a .arg-group class in styles.css, but I don't think we have a use for it at the moment. I'll take it out of the README

davemfish · 2021-01-14T21:18:48Z

tests/app.test.js

+    fileRegistry.INVEST_UI_DATA, `${spec.module}.json`
+  );
+  fs.writeFileSync(uiSpecFilePath, JSON.stringify(uiSpec));
+


Will these files get cleaned up after the tests? We might want a beforeAll and afterAll functions to take care of that.

Moved the writeFileSync into beforeAll and added unlinkSync to afterAll

davemfish · 2021-01-14T21:28:52Z

tests/setuptab.test.js

@@ -340,18 +349,27 @@ describe('UI spec functionality', () => {
      args: {
        controller: {
          name: 'afoo',
-          type: 'csv',
-          ui_control: ['arg2'],
+          type: 'csv'


A few lines above where I can't leave a comment, there's a code comment that I think we can get rid of now.

emlys · 2021-01-21T00:43:04Z

Thanks for catching those miscellaneous things @davemfish. I agree some kind of validation would be good. Since a lot is changing in #84 as well I'd wait until both PRs are done. In general, I like your idea of clicking on each model to make sure it loads.

emlys · 2021-01-21T18:34:07Z

The failing windows test error looks like this one: electron/electron#25267

emlys · 2021-01-25T16:44:35Z

Closing because all changes have been merged into #84.

emlys added 4 commits January 11, 2021 17:32

refactor UI spec order into its own section

aea8262

rename to argsOrder; separate argsValidation

58f323d

use props.uiSpec.order everywhere

fdba26d

remove log statements; add example carbon JSON

237e21d

emlys marked this pull request as draft January 12, 2021 01:46

emlys commented Jan 12, 2021

View reviewed changes

emlys requested a review from davemfish January 12, 2021 02:02

emlys marked this pull request as ready for review January 12, 2021 02:02

emlys changed the title ~~Task/refactor UI spec~~ Refactor UI spec "order" property Jan 12, 2021

davemfish suggested changes Jan 12, 2021

View reviewed changes

emlys added 10 commits January 12, 2021 14:16

update some tests, rename args to argsOptions, make optional

f592684

rename ui_option to control_option

4f0c784

update more tests

0f6f948

finalize setuptab tests

5302c73

reformat some ui data JSONs

01ecc60

UI spec README explaining the format

af7c696

fix markdown formatting

0d133dd

even more ui spec files

0630e61

more more UI spec files

5e5069c

the last of the ui spec files

64e81fd

davemfish suggested changes Jan 14, 2021

View reviewed changes

addressing pr comments

6155b74

emlys added 2 commits January 20, 2021 17:45

correct variable name

ff3dc16

destructure this.props

88ae7d9

emlys closed this Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor UI spec "order" property #79

Refactor UI spec "order" property #79

emlys commented Jan 12, 2021 •

edited

emlys Jan 12, 2021

davemfish left a comment

davemfish Jan 12, 2021

emlys Jan 12, 2021

emlys commented Jan 12, 2021

davemfish left a comment

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

davemfish Jan 14, 2021

emlys Jan 21, 2021

davemfish Jan 14, 2021

emlys Jan 21, 2021

davemfish Jan 14, 2021

emlys commented Jan 21, 2021

emlys commented Jan 21, 2021

emlys commented Jan 25, 2021

Refactor UI spec "order" property #79

Refactor UI spec "order" property #79

Conversation

emlys commented Jan 12, 2021 • edited

Choose a reason for hiding this comment

davemfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlys commented Jan 12, 2021

davemfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlys commented Jan 21, 2021

emlys commented Jan 21, 2021

emlys commented Jan 25, 2021

emlys commented Jan 12, 2021 •

edited