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.

Sign up for GitHub

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

Jump to bottom

WIP -- Value Def Customization UI #827

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

WIP -- Value Def Customization UI #827

wants to merge 18 commits into from

Conversation

ssharif6
Copy link
Contributor

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

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

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
@@ -5,6 +5,7 @@ import * as CSSModules from 'react-css-modules';
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

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
@@ -91,27 +93,35 @@ class EncodingShelfBase extends React.PureComponent<
attachment="top left"
targetAttachment="bottom left"
>
{(fieldDef && !isWildcardChannelId(id) && contains(CUSTOMIZABLE_ENCODING_CHANNELS, id.channel)) ?
{(!isWildcardChannelId(id) && contains(CUSTOMIZABLE_ENCODING_CHANNELS, id.channel)) ?
Copy link
Member

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

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
@@ -1,6 +1,7 @@

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

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