Custom Wildcard Field UI #722

espressoroaster · 2017-09-15T01:13:51Z

kanitw · 2017-09-15T02:05:00Z

src/reducers/custom-wildcard-field.test.ts

-      const noCustomWildcardFields: CustomWildcardField[] = [];
-      const customWildcardFields = customWildcardFieldReducer(
-        noCustomWildcardFields,
+      const noCustomWildcardFieldDefs: CustomWildcardFieldDef[] = [];


Just use [], the name is unnecessary and is a bit confusing the first time I saw it.

kanitw · 2017-09-15T02:06:39Z

src/components/data-pane/wildcard-field-dropper.tsx

+      if (isWildcard(fieldDef.field)) {
+        if (fieldDef.field === '?') {
+          fields = schema.fieldNames()
+                          .filter(field => schema.vlType(field) === type);


I would just tab by two empty space. (The current tabbing isn't aligned anyway)

kanitw · 2017-09-15T02:15:15Z

src/components/data-pane/field-list.tsx

+
+    let field;
+    let popupComponent;
+    let isCustomWildcardField;


Why do you need let for isCustomWildcardField? You can just use const here.

kanitw · 2017-09-15T02:15:28Z

src/components/data-pane/field-list.tsx

+    const key = isWildcard(fieldDef.field) ? stringify(fieldDef) : fieldDef.field;
+
+    let field;
+    let popupComponent;


For popupComponent, you may want to extract a method.

kanitw · 2017-09-15T02:16:02Z

src/components/data-pane/field-list.tsx

+    const {schema, filters, handleAction} = this.props;
+    const key = isWildcard(fieldDef.field) ? stringify(fieldDef) : fieldDef.field;
+
+    let field;


Why do you need let for field? Just use const if possible.

kanitw · 2017-09-15T02:16:30Z

src/components/data-pane/field-list.tsx


-  private renderField(fieldDef: ShelfFieldDef, popupComponent?: JSX.Element) {
-    const {schema, filters, handleAction} = this.props;
    let filterShow;


use const for filterShow

kanitw · 2017-09-15T02:17:15Z

src/components/data-pane/index.tsx

  config: VoyagerConfig;
+  customWildcardFieldDefs: CustomWildcardFieldDef[];


This doesn't seem to be used anywhere?

kanitw · 2017-09-15T02:18:11Z

src/components/field/index.tsx

@@ -48,7 +49,8 @@ export interface FieldPropsBase {
  /** Remove field event handler.  If not provided, remove button will disappear. */
  onRemove?: () => void;

-  handleAction?: (action: FilterAction | ShelfAction | DatasetSchemaChangeFieldType) => void;
+  handleAction?: (action: CustomWildcardAction | FilterAction |


Better make it implement ActionHandler?

kanitw · 2017-09-15T02:23:56Z

src/components/data-pane/wildcard-field-drop-zone.tsx

+
+export interface CustomWildcardFieldDropZonePropsBase {
+  schema: Schema;
+  handleAction?: (action: CustomWildcardAction) => void;


Why is this optional? Better just make it implements ActionHandler

kanitw · 2017-09-15T02:24:17Z

src/components/data-pane/wildcard-field-dropper.tsx

+  customWildcardField: CustomWildcardFieldDef;
+  schema: Schema;
+  index: number;
+  handleAction?: (action: CustomWildcardAction) => void;


Why is this optional? Better just make it implements ActionHandler

kanitw · 2017-09-15T02:24:57Z

src/components/data-pane/wildcard-field-drop-zone.tsx

+
+    let fields;
+    if (isWildcard(fieldDef.field)) {
+      if (fieldDef.field === '?') {


Use SHORT_WILDCARD over question mark

kanitw · 2017-09-15T02:25:47Z

src/components/data-pane/wildcard-field-drop-zone.tsx

+        fields = schema.fieldNames()
+                        .filter(field => schema.vlType(field) === type);
+      } else {
+        fields = fieldDef.field.enum.concat([]);


Why concat []?

kanitw · 2017-09-15T02:26:17Z

src/components/data-pane/wildcard-field-drop-zone.tsx

+      return;
+    }
+
+    const {fieldDef} = monitor.getItem() as DraggedFieldIdentifier;


Why do we need as here?

Bad type inference. Monitor.getItem() is inferred simply as type 'Object' -- it doesn't know the object contains the property fieldDef. I casted to DraggedFieldIdentifier so it could infer fieldDef and let me write const {fieldDef} =. We do the same thing in encoding-shelf.tsx for the encodingShelfTarget.

kanitw · 2017-09-15T02:29:24Z

src/components/data-pane/wildcard-field-editor.tsx

+    });
+  }
+
+  private handleDescriptionChange(event: any) {


onDescriptionChange

kanitw · 2017-09-15T02:29:35Z

src/components/data-pane/wildcard-field-editor.tsx

+    );
+  }
+
+  protected customWildcardRemoveField(field: string) {


Use consistent name onRemoveField (we use on... elsewhere in the project)

(customWildcardRemoveField isn't consistent with handleDescriptionChange.)

kanitw · 2017-09-15T02:31:20Z

src/components/data-pane/wildcard-field-dropper.tsx

+    return connectDropTarget(this.props.children[0]);
+  }
+
+  private customWildcardRemove() {


Why do you need this remove here?

espressoroaster · 2018-02-11T06:23:04Z

cc: @kanitw

kanitw · 2018-03-04T04:21:51Z

Is this ready? If so, rebase?

CustomWildcardField ==> CustomWildcardFieldDef

Trying to make Field both a dragsource and droptarget

espressoroaster · 2018-03-09T20:16:37Z

cc @kanitw Rebased.

kanitw · 2018-05-18T17:14:11Z

There is a conflict -- do you want to rebase and squash to one commit (or a few commits that are more useful?)

kanitw · 2018-05-18T17:16:43Z

src/components/data-pane/field-list.tsx

+            fields = schema.fieldNames()
+                       .filter(field => schema.vlType(field) === type);
+          } else {
+            fields = droppedFieldDef.field.enum.concat([]);


Why concat empty string?

kanitw · 2018-05-18T17:19:59Z

src/components/data-pane/field-list.tsx

+  (state: State) => {
+    return {
+      // Somehow TS does not infer type that CustomWildcardFieldDefs can be a ShelfFieldDef
+      fieldDefs: selectCustomWildcardFieldDefs(state) as ShelfFieldDef[],


There are so too casting in this PR -- I doubt if we really need all of them.

Can you see if upgrading to the latest TS allow you to eliminate a lot of these as?

kanitw · 2018-05-18T17:21:56Z

src/components/field/index.tsx

+
+    if (isCustomWildcardField) {
+      const fields = (fieldDef as CustomWildcardFieldDef).field.enum; // TS isn't the inferring correct type
+      customWildcardFieldDescription = "(" + fields.length + ") " + (description || fields.toString());


fields.toString()

This will add comma without space -- is that what you want?
Wouldn't it be better to .join(', ')?

kanitw · 2018-05-18T17:22:19Z

src/components/field/index.tsx

@@ -109,32 +124,54 @@ class FieldBase extends React.PureComponent<FieldProps, FieldState> {
      fnName = fn;
    }

-    const component = (
+    const isCustomWildcardField = isWildcardDef(fieldDef.field);
+    let customWildcardFieldDescription = "";


Please always use single quote for TS code. (Do the same for the whole PR)

kanitw · 2018-05-18T17:23:13Z

src/models/custom-wildcard-field.ts

 import {ShelfFieldDef} from './shelf/spec/encoding';

-export type CustomWildcardField = {
-  fields: string[];
+export type CustomWildcardFieldDef = {


Rename the file accordingly?

kanitw · 2018-05-18T17:29:34Z

src/components/data-pane/wildcard-field-drop-zone.scss

@@ -0,0 +1,23 @@
+.drop-zone {


Instead of declaring a totally new CSS, can this extend some style of Field

The old version of Voyager has nicer CSS here

We should rounded corner like normal Field

It should be semi-transparent

espressoroaster requested a review from kanitw September 15, 2017 01:13

espressoroaster added the In Progress label Sep 15, 2017

kanitw requested changes Sep 15, 2017

View reviewed changes

espressoroaster force-pushed the fo/custom-wildcard-field-UI branch from a892f0c to 81425d7 Compare September 15, 2017 22:02

espressoroaster force-pushed the fo/custom-wildcard-field-UI branch from 1c5b259 to 187272a Compare February 2, 2018 18:50

espressoroaster force-pushed the fo/custom-wildcard-field-UI branch from 187272a to 9ec6fb1 Compare February 11, 2018 05:28

espressoroaster changed the title ~~[WIP] Custom Wildcard Field UI~~ Custom Wildcard Field UI Feb 11, 2018

espressoroaster force-pushed the fo/custom-wildcard-field-UI branch from 92d5004 to 8015021 Compare February 11, 2018 06:27

espressoroaster and others added 19 commits March 9, 2018 11:58

Update model, reducers, selectors, and tests

52d1cf7

CustomWildcardField ==> CustomWildcardFieldDef

Add wildcard-field-editor component

2bacaf4

Add wildcard-field-dropper component to wrap CustomWildcardField in list

23df761

Add wildcard-field-drop-zone component

5ac40cd

Refactor field-list and add CustomWildcardFieldList

8a42aa8

Add CustomWildcardFieldList and DropZone to DataPane component

97e8665

Add TODO to remove handle action from Field

9f73ca6

UPDATE THIS COMMIT LATER -- THIS CSS ONLY EXISTS SO VOYAGER WILL COMPILE

5c4637f

Add missing dispatch handler to data pane

db4a6b1

Make small fixes

fb0e53d

WIP Field / Field-List changes

fc6e74e

Trying to make Field both a dragsource and droptarget

the right high-level component

efd1071

Move onDrop logic out of Field since it's a dumb component

a531af5

Add CSS and canDrop monitor

d638fc1

Remove wildcard field dropper

5da5a02

Add can-drop and is-over CSS for hovering over custom wildcard field

30e124f

Remove unnecessary concat []

03c0edc

Fix selector import

b62d6e8

Fix custom wildcard field colors if canDrop and onHover

deb2c27

espressoroaster added 8 commits March 9, 2018 12:00

Fix wildcard field drop zone text and css

32dba89

Add title to Custom Wildcard Fields

8e6dec5

Add Delete Wildcard button to custom wildcard popup

3ccc3de

Remove remove button from custom wildcard field pills in field list

f22c1ae

Fix margin for wildcard field drop zone

16466e6

Fix isOver + canDrop logic

f3b3b6d

Add comment to explain type cast

8ea9ee7

Change customWildcardFields to customWildcardFieldDefs during rebase

aef4b0f

espressoroaster force-pushed the fo/custom-wildcard-field-UI branch from 8015021 to aef4b0f Compare March 9, 2018 20:16

espressoroaster removed the In Progress label May 1, 2018

ssharif6 force-pushed the master branch from 6e335ea to 3521d78 Compare May 12, 2018 04:13

kanitw requested changes May 18, 2018

View reviewed changes

kanitw reviewed May 18, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom Wildcard Field UI #722

Custom Wildcard Field UI #722

espressoroaster commented Sep 15, 2017 •

edited

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

espressoroaster Sep 29, 2017 •

edited

kanitw Sep 15, 2017

kanitw Sep 15, 2017

kanitw Sep 15, 2017

espressoroaster commented Feb 11, 2018

kanitw commented Mar 4, 2018

espressoroaster commented Mar 9, 2018 •

edited

kanitw commented May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

kanitw May 18, 2018

		config: VoyagerConfig;
		customWildcardFieldDefs: CustomWildcardFieldDef[];

Custom Wildcard Field UI #722

Are you sure you want to change the base?

Custom Wildcard Field UI #722

Conversation

espressoroaster commented Sep 15, 2017 • edited

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

espressoroaster Sep 29, 2017 • edited

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

espressoroaster commented Feb 11, 2018

kanitw commented Mar 4, 2018

espressoroaster commented Mar 9, 2018 • edited

kanitw commented May 18, 2018

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

espressoroaster commented Sep 15, 2017 •

edited

espressoroaster Sep 29, 2017 •

edited

espressoroaster commented Mar 9, 2018 •

edited