Skip to content

WIP -- Value Def Customization UI#827

Open
ssharif6 wants to merge 18 commits intomasterfrom
ss/value-def-ui
Open

WIP -- Value Def Customization UI#827
ssharif6 wants to merge 18 commits intomasterfrom
ss/value-def-ui

Conversation

@ssharif6
Copy link
Contributor

@ssharif6 ssharif6 commented Jul 23, 2018

UI for customization of constant values (valuedef). Addresses issue #662.

@ssharif6 ssharif6 requested a review from kanitw July 23, 2018 15:23
@ssharif6 ssharif6 changed the title Ss/value def ui Value Def Customization Jul 23, 2018
@ssharif6 ssharif6 changed the title Value Def Customization Value Def Customization UI Jul 23, 2018
@ssharif6 ssharif6 self-assigned this Jul 23, 2018
@kanitw
Copy link
Member

kanitw commented Jul 23, 2018 •
edited by ssharif6
Loading

  • There is a bug in CompassQL for size value. (Putting quantitative X normally get ticks, but adding size value change ticks to points.)

Similarly, bars got changed to points too:

image

And line got changed to area:

image

In addition to fixing these, perhaps, it's worth trying more different combination of theses.

  • The Value def UI is not styled yet. Better use consistent styles with fieldDef customizer. For example, text could look like axis title.

image

  • shape value customizer should be enabled only if the mark is point, text value customizer should be enabled only if the mark is text

  • For all of these values, there is no way to set "use default" value.

  • For text, if I type something and then later delete the text, then I get weird empty fieldDef.

image

  • Size of point, text, and bar use different units (pixel area, font pixel, and width pixel -- we have to think about how we gonna make size work without causing problem to other marks.

kanitw
kanitw reviewed Jul 23, 2018
View reviewed changes
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

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

Looks like you import node_modules in many places. Consider 1) use Typescript Importer if you haven't
2) check the project config if there is something causing this wrong imports.

src/components/encoding-pane/encoding-shelf.tsx Outdated
/>
</div>
<div ref={this.popupRefHandler}>
{ (!fieldDef && !valueDef) || isValueDef(valueDef) ?
Copy link
Member

@kanitw kanitw Jul 23, 2018

Choose a reason for hiding this comment

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

Why not just !fieldDef instead of (!fieldDef && !valueDef) || isValueDef(valueDef) ?

Copy link
Contributor Author

@ssharif6 ssharif6 Jul 25, 2018

Choose a reason for hiding this comment

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

Fixed! The issue was here: https://github.com/vega/voyager/blob/master/src/components/encoding-pane/index.tsx#L117

I fixed this by adding appropriate typeguarding and the basic !fieldDef logic can now work correctly

src/components/encoding-pane/encoding-shelf.tsx Outdated
import {ConnectDropTarget, DropTarget, DropTargetCollector, DropTargetSpec} from 'react-dnd';
import * as TetherComponent from 'react-tether';
import {contains} from 'vega-lite/build/src/util';
import {isValueDef} from '../../../node_modules/vega-lite/build/src/fielddef';
Copy link
Member

@kanitw kanitw Jul 23, 2018

Choose a reason for hiding this comment

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

Remove ../../../node_modules/. Please also check if you have accidentally included node_modules like this in other places too. :)

src/components/encoding-pane/encoding-shelf.tsx Outdated
targetAttachment="bottom left"
>
{(fieldDef && !isWildcardChannelId(id) && contains(CUSTOMIZABLE_ENCODING_CHANNELS, id.channel)) ?
{(!isWildcardChannelId(id) && contains(CUSTOMIZABLE_ENCODING_CHANNELS, id.channel)) ?
Copy link
Member

@kanitw kanitw Jul 23, 2018

Choose a reason for hiding this comment

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

Since we don't allow customizing x and y valuedef, caret shouldn't be shown for x and y if there is no fieldDef.

src/components/encoding-pane/value-customizer.tsx Outdated
}

protected changeValue(result: any) {
const value = result.formData[Object.keys(result.formData)[0]].toString();
Copy link
Member

@kanitw kanitw Jul 23, 2018

Choose a reason for hiding this comment

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

why toString(). Isn't size not a string, for example?

src/queries/alternative-encodings.ts Outdated

import {Query} from 'compassql/build/src/query/query';
import {isWildcard, SHORT_WILDCARD} from 'compassql/build/src/wildcard';
import {isValueQuery} from '../../node_modules/compassql/build/src/query/encoding';
Copy link
Member

@kanitw kanitw Jul 23, 2018

Choose a reason for hiding this comment

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

node_modules

@ssharif6 ssharif6 changed the title Value Def Customization UI WIP -- Value Def Customization UI Jul 23, 2018
@ssharif6 ssharif6 mentioned this pull request Aug 1, 2018
Merged
@ssharif6
Add mainspec prop to read mark from main view
5da835b 
Remove ../node_modules prefix


asdf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kanitw kanitw kanitw left review comments

Assignees

@ssharif6 ssharif6

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ssharif6 @kanitw