Release 1.8.0-RC1 back into develop #94

MarcusBaitz · 2018-03-26T07:03:06Z

No description provided.

* Replace concept nodes by concept lists (#88) * Make concept lists the default node type, remove special handling * Add code list resolution to demo data * initial money range filter * update moneyRange * remove accounting update yarn.lock * update demo data * update formatted value for money range * fix decimal scale expect number

rs22

I've left a few notes.

Because these are a lot of changes, it's a little weird to review the money filter feature after it was already committed onto a release branch -- I'd prefer if we'd review it isolated from other changes, then merge into develop, then merge develop into release, or, if that is not desired, attempt a cherry-pick of the merged feature and apply it onto the release branch.

rs22 · 2018-03-26T11:22:09Z

frontend/app/api/concepts.json

              "unit": "1K USD",
              "description": "The movie's total budget",
-              "min": 1
+              "min": 1,
+              "value": {


When the backend returns concept trees they don't have a value property.

rs22 · 2018-03-26T11:27:00Z

frontend/app/api/concepts.json

+                "exact": 9999
+              },
+              "defaultValue": {
+                "min": 4455,


These values seem to be quite arbitrary. I'd suggest we set them as value in the stored-queries/25.json -- otherwise people might wonder what they mean when they try out the demo.

Also, let's be strict about data types here. The backend always sets these values as numbers of cents in the JSON object so we should follow this in the demo data, too.

Additionally, it should never occur that min/max and exact are set at the same time.

rs22 · 2018-03-26T11:33:33Z

frontend/lib/js/form-components/ClearableInput.js

+            className="clearable-input__input"
+            placeholder={props.placeholder}
+            type={props.inputType}
+            onValueChange={(values) => { // values: {floatValue, formattedValue, value}


Instead of providing this comment, we can type-annotate value directly, i.e.

onValueChange={(values: {floatValue: number, formattedValue: string, value: string}) => {

rs22 · 2018-03-26T11:54:34Z

frontend/lib/js/form-components/InputRange.js

  // Make sure undefined / null is never set as a value, but an empty string instead
  const minValue = (value && value.min) || '';
  const maxValue = (value && value.max) || '';
  const exactValue = (value && value.exact) || '';
+  const minFormattedValue = ((formattedValue && formattedValue.min) || minValue) || '';
+  const maxFormattedValue = ((formattedValue && formattedValue.max) || maxValue) || '';
+  const exactFormattedValue = ((formattedValue && formattedValue.exact) || exactValue) || '';


I think you have to divide minValue / maxValue / exactValue by 100 at this point -- those are provided as cent values.

rs22 · 2018-03-26T12:00:56Z

frontend/lib/js/form-components/ClearableInput.js


 import { isEmpty }          from '../common/helpers';
+import { MONEY_RANGE }      from '.';


Maybe a question of taste -- import {} from '.' is not very descriptive, we could also import this directly from './filterTypes'

rs22 · 2018-03-26T12:11:54Z

frontend/lib/js/form-components/InputRange.js

+        {
+          min: props.input.formattedValue ? props.input.formattedValue.min : null,
+          max: props.input.formattedValue ? props.input.formattedValue.max : null,
+          [type]: formattedValue


I don't know why -- but when I was testing, the x-Buttons inside the input elements didn't have the expected effect.

When resetting in exact mode, nothing happens, when in range-mode, only the formattedValue is reset but not the underlying raw value.

Deleting the values in exact or the ranges was generally a problem. Should be correct now

rs22 · 2018-03-26T12:13:08Z

frontend/app/api/concepts.json

@@ -59,10 +59,20 @@
            {
              "id": "budget",
              "label": "Budget",
-              "type": "INTEGER_RANGE",
+              "type": "MONEY_RANGE",
              "unit": "1K USD",


This doesn't make sense when we're now entering Euro values.

This will render the correct filter type.

I meant the "unit": "1K USD" line

rs22 · 2018-03-26T12:16:28Z

frontend/lib/js/form-components/filterTypes.js

@@ -14,6 +15,7 @@ export const SUPPORTED_FILTERS = {
  REAL_RANGE: REAL_RANGE,
  STRING: STRING,
  BIG_MULTI_SELECT: BIG_MULTI_SELECT,
+  MONEY_RANGE: MONEY_RANGE,
  // Maybe later:
  // 'MONEY_RANGE'


Nitpick: Could you remove this comment line, now that we have a MONEY_RANGE? Could you also add MONEY_RANGE below REAL_RANGE above, so that all range filter types are grouped together?

rs22 · 2018-03-26T12:17:47Z

frontend/lib/js/query-node-editor/ParameterTableFilters.js

@@ -120,7 +121,6 @@ const ParameterTableFilters = (props: PropsType) => (
                    inputType="number"
                    input={{
                      value: filter.value,
-                      defaultValue: filter.defaultValue,


Why did you remove this (same goes for INTEGER_RANGE above)?

I ask myself the same thing. I did not consciously remove that.

MarcusBaitz · 2018-03-26T14:12:04Z

I had a branch on money range open, but by making a new delivery tomorrow, I've already merged the feature branch into the release branch.

rs22 · 2018-03-26T14:46:44Z

frontend/.eslintrc.json

@@ -24,7 +24,7 @@
    "comma-dangle": 0, // May be there, helps better diffs and easier maintainability
    "indent": 0,                 // We indent how we like. (sometimes a custom indentation is great)
    "jsx-quotes": [2, "prefer-double"],
-    "max-len": [2, 100],
+    "max-len": [2, 120],


Was there a reason to change this?

The width of the monitors or the current font sizes allow this. The value 150 would also go ;) if you do not mind I would leave it like that.

rs22 · 2018-03-26T14:48:22Z

frontend/app/api/concepts.json

              "unit": "1K USD",
              "description": "The movie's total budget",
-              "min": 1
+              "min": 1,
+              "defaultValue": {


I still think we should rather demo this in the example stored query, instead of always settings this default value.
What movie has a budget smaller than 44,55 Euros? 😉
Also, the value for exact has to be removed.

prepare release 1.8.0-RC1

e3e8722

MarcusBaitz requested a review from rs22 March 26, 2018 07:03

MarcusBaitz added 3 commits March 26, 2018 09:03

Merge branch 'develop' into release/1.8.0

7522138

fixed wrong result display position (#93)

8bd637e

rs22 suggested changes Mar 26, 2018

View reviewed changes

update money range

ecfab19

MarcusBaitz and others added 2 commits March 26, 2018 16:14

Merge branch 'develop' into release/1.8.0

ec9e32c

fix type imports

ec486ca

rs22 suggested changes Mar 26, 2018

View reviewed changes

Marcus Baitz added 2 commits March 26, 2018 18:52

update demo data

15967b5

reset max-len in eslintrc.json

c6971f7

rs22 approved these changes Mar 26, 2018

View reviewed changes

rs22 merged commit 75912f9 into develop Mar 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Release 1.8.0-RC1 back into develop #94

Release 1.8.0-RC1 back into develop #94

MarcusBaitz commented Mar 26, 2018

rs22 left a comment

rs22 Mar 26, 2018

rs22 Mar 26, 2018

rs22 Mar 26, 2018

rs22 Mar 26, 2018

rs22 Mar 26, 2018

rs22 Mar 26, 2018

MarcusBaitz Mar 26, 2018 •

edited

rs22 Mar 26, 2018

MarcusBaitz Mar 26, 2018

rs22 Mar 26, 2018 •

edited

rs22 Mar 26, 2018

rs22 Mar 26, 2018 •

edited

MarcusBaitz Mar 26, 2018

MarcusBaitz commented Mar 26, 2018

rs22 Mar 26, 2018

MarcusBaitz Mar 26, 2018 •

edited

rs22 Mar 26, 2018


		import { isEmpty } from '../common/helpers';
		import { MONEY_RANGE } from '.';

Release 1.8.0-RC1 back into develop #94

Release 1.8.0-RC1 back into develop #94

Conversation

MarcusBaitz commented Mar 26, 2018

rs22 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

MarcusBaitz Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rs22 Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rs22 Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusBaitz commented Mar 26, 2018

Choose a reason for hiding this comment

MarcusBaitz Mar 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcusBaitz Mar 26, 2018 •

edited

rs22 Mar 26, 2018 •

edited

rs22 Mar 26, 2018 •

edited

MarcusBaitz Mar 26, 2018 •

edited