Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom Wildcard Field UI #722

Open
wants to merge 27 commits into
base: master
Choose a base branch
from

Conversation

espressoroaster
Copy link
Member

@espressoroaster espressoroaster commented Sep 15, 2017

Fix #555

custom_wildcard_fields

const noCustomWildcardFields: CustomWildcardField[] = [];
const customWildcardFields = customWildcardFieldReducer(
noCustomWildcardFields,
const noCustomWildcardFieldDefs: CustomWildcardFieldDef[] = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


let field;
let popupComponent;
let isCustomWildcardField;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

let field;
let popupComponent;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For popupComponent, you may want to extract a method.

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

let field;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use const for filterShow

config: VoyagerConfig;
customWildcardFieldDefs: CustomWildcardFieldDef[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to be used anywhere?

@@ -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 |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better make it implement ActionHandler?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this optional? Better just make it implements ActionHandler

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this optional? Better just make it implements ActionHandler


let fields;
if (isWildcard(fieldDef.field)) {
if (fieldDef.field === '?') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use SHORT_WILDCARD over question mark

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why concat []?

return;
}

const {fieldDef} = monitor.getItem() as DraggedFieldIdentifier;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need as here?

Copy link
Member Author

@espressoroaster espressoroaster Sep 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
}

private handleDescriptionChange(event: any) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onDescriptionChange

);
}

protected customWildcardRemoveField(field: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(customWildcardRemoveField isn't consistent with handleDescriptionChange.)

return connectDropTarget(this.props.children[0]);
}

private customWildcardRemove() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this remove here?

@espressoroaster
Copy link
Member Author

cc: @kanitw

custom_wildcard_fields

@espressoroaster espressoroaster changed the title [WIP] Custom Wildcard Field UI Custom Wildcard Field UI Feb 11, 2018
@kanitw
Copy link
Member

kanitw commented Mar 4, 2018

Is this ready? If so, rebase?

@espressoroaster
Copy link
Member Author

espressoroaster commented Mar 9, 2018

cc @kanitw Rebased.

@kanitw
Copy link
Member

kanitw commented May 18, 2018

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why concat empty string?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fields.toString()

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

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

const component = (
const isCustomWildcardField = isWildcardDef(fieldDef.field);
let customWildcardFieldDescription = "";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

export type CustomWildcardField = {
fields: string[];
export type CustomWildcardFieldDef = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename the file accordingly?

@@ -0,0 +1,23 @@
.drop-zone {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  2. The old version of Voyager has nicer CSS here

  • We should rounded corner like normal Field
  • It should be semi-transparent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants