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

Support href and cursor properties for marks #3229

Merged
merged 4 commits into from
Jan 15, 2018
Merged

Support href and cursor properties for marks #3229

merged 4 commits into from
Jan 15, 2018

Conversation

domoritz
Copy link
Member

@domoritz domoritz commented Dec 9, 2017

No description provided.

@domoritz domoritz requested a review from kanitw December 9, 2017 11:31
@domoritz domoritz force-pushed the dom/href branch 2 times, most recently from 99410fd to 091d051 Compare December 9, 2017 11:43
/**
* The mouse cursor used over the mark. Any valid [CSS cursor type](https://developer.mozilla.org/en-US/docs/Web/CSS/cursor#Values) can be used.
*/
cursor?: string;
Copy link
Member

Choose a reason for hiding this comment

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

The link you provided lists all cursor type. Perhaps, make it an enum here?

auto | default | none | context-menu | help | pointer | progress | wait | cell | crosshair | text | vertical-text | alias | copy | move | no-drop | not-allowed | e-resize | n-resize | ne-resize | nw-resize | s-resize | se-resize | sw-resize | w-resize | ew-resize | ns-resize | nesw-resize | nwse-resize | col-resize | row-resize | all-scroll | zoom-in | zoom-out | grab | grabbing

Copy link
Member Author

@domoritz domoritz Dec 26, 2017

Choose a reason for hiding this comment

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

Okay. I don't know whether browser vendors will add more in the future, though.

fontStyle: 1
fontStyle: 1,
href: 1,
cursor: 1,
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why previously list cursor as "vg channel that do not have mark config". Can you test if cursor works below for sure?

Btw, does it really make sense to have href as a mark config too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Working on it

Copy link
Member Author

Choose a reason for hiding this comment

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

{
  "$schema": "https://vega.github.io/schema/vega-lite/v2.json",
  "description": "A simple bar chart with embedded data.",
  "data": {
    "values": [
      {"a": "A","b": 28}, {"a": "B","b": 55}, {"a": "C","b": 43},
      {"a": "D","b": 91}, {"a": "E","b": 81}, {"a": "F","b": 53},
      {"a": "G","b": 19}, {"a": "H","b": 87}, {"a": "I","b": 52}
    ]
  },
  "mark": "bar",
  "encoding": {
    "x": {"field": "a", "type": "ordinal"},
    "y": {"field": "b", "type": "quantitative"}
  },
  "config": {
    "bar": {
      "cursor": "pointer"
    }
  }
}

works

### Stroke Style

{% include table.html props="strokeWidth,strokeDash,strokeDashOffset" source="MarkConfig" %}

### Link Properties

Marks can act as hyperlinks when the `href` property is defined.
Copy link
Member

Choose a reason for hiding this comment

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

  • href property -> href channel

  • Maybe also add:

A cursor channel can also be provided to serve as affordance for the links.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@domoritz
Copy link
Member Author

I will finish this in a few days. Thanks for the review.

@domoritz
Copy link
Member Author

domoritz commented Dec 30, 2017

Ready. Note that this PR does not add href or cursor as a channel.

### Stroke Style

{% include table.html props="strokeWidth,strokeDash,strokeDashOffset" source="MarkConfig" %}

### Link Properties

Marks can act as hyperlinks when the `href` channel is defined. A `cursor` channel can also be provided to serve as affordance for the links.
Copy link
Member

Choose a reason for hiding this comment

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

If you don't add them as channels, why the docs say that they are channels here?

Copy link
Member Author

@domoritz domoritz Jan 1, 2018

Choose a reason for hiding this comment

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

Ups. Fixed.

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.

I still feel like this is quite weird to merge.
We better support them as encoding channels.

@domoritz domoritz force-pushed the dom/href branch 2 times, most recently from afb4d0c to a832c36 Compare January 11, 2018 17:03
@@ -180,27 +180,27 @@ In addition to the constant `value`, [value definitions](#value-def) of mark pro
See [the `condition`](condition.html) page for examples how to specify condition logic.

{:#text}
## Text and Tooltip Channels
## Text, Tooltip, and HREF Channels
Copy link
Member

@kanitw kanitw Jan 12, 2018

Choose a reason for hiding this comment

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

href should be on its own category -- it should not have format property

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I was thinking that one might want to format a number to be a URL like www.site.com?page=${id}.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah but having format wouldn't help you do that anyway?

https://github.com/d3/d3-format#locale_format

Copy link
Member

Choose a reason for hiding this comment

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

oh I see -- you can use "fill"

Copy link
Member

Choose a reason for hiding this comment

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

Actually you can't .. fill is for a slightly different purpose

Copy link
Member Author

@domoritz domoritz Jan 12, 2018

Choose a reason for hiding this comment

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

You're right. I was thinking of dates where you can do something like foo %H bar. However, I think that's a rare case and can be addressed with a formula.

src/encoding.ts Outdated
/**
* A URL to load upon mouse click.
*/
href?: FieldDefWithCondition<TextFieldDef<F>> | ValueDefWithCondition<TextFieldDef<F>>;
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be TextFieldDef?

Copy link
Member Author

Choose a reason for hiding this comment

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

What if you want a conditional url?

Copy link
Member

Choose a reason for hiding this comment

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

Well, TextFieldDef and conditional are orthogonal. TextFieldDef has format that href shouldn't

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed and rebased.

{:#href}
## Hyperlink Channel

By setting the `href` channel, a mark becomes a hyperlink. The specified URL is loaded upon a muse click. The `cursor` mark property can be set to `pointer` to serve as affordance for hyperlinks.
Copy link
Member

Choose a reason for hiding this comment

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

add a link for "The cursor mark property"?

### Stroke Style

{% include table.html props="strokeWidth,strokeDash,strokeDashOffset" source="MarkConfig" %}

### Link Properties

Marks can act as hyperlinks when the `href` property is defined. A `cursor` property can also be provided to serve as affordance for the links.
Copy link
Member

Choose a reason for hiding this comment

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

href property or channel

(add link for "channel" part)

src/channel.ts Outdated
// text and tooltip has format instead of scale
text: _t, tooltip: _tt,
// x2 and y2 share the same scale as x and y
// text, tooltip, and href have format instead of scale
Copy link
Member

Choose a reason for hiding this comment

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

href does not have format

@@ -120,6 +120,21 @@ describe('Mark', function() {
assert.equal(markGroup[0].encode.update.tooltip.value, 'foo');
});
});

describe('Bar with href', () => {
Copy link
Member

Choose a reason for hiding this comment

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

So far, I have created test for these types of basic test in point.

Also, going forward, let's try to use describe for function and namespace rather than cases (which it is more appropriate).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I'm moving the tooltip and href tests.

@kanitw kanitw mentioned this pull request Jan 15, 2018
5 tasks
@domoritz
Copy link
Member Author

Done with revisions

src/channel.ts Outdated
@@ -128,7 +128,8 @@ export type PositionScaleChannel = typeof POSITION_SCALE_CHANNELS[0];
// NON_POSITION_SCALE_CHANNEL = SCALE_CHANNELS without X, Y
const {
// x2 and y2 share the same scale as x and y
// text, tooltip, and href have format instead of scale
// text and tooltip have format instead of scale,
// href has neother format, nor scale
Copy link
Member

Choose a reason for hiding this comment

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

neither

@kanitw kanitw merged commit b4dfb96 into master Jan 15, 2018
@kanitw kanitw deleted the dom/href branch January 15, 2018 05:25
@domoritz
Copy link
Member Author

cc @rdeline

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