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

add new channel opacity #1336

Merged
merged 17 commits into from
May 10, 2016
Merged

add new channel opacity #1336

merged 17 commits into from
May 10, 2016

Conversation

YuhanLu
Copy link
Contributor

@YuhanLu YuhanLu commented May 3, 2016 •
edited

Fix #1025
redo #1315
Add a new encoding channel opacity (with an example and doc).
Support stack for opacity.

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 3, 2016

It is ready for review

@kanitw
Copy link
Member

kanitw commented May 8, 2016

@YuhanLu value property should also work too.

{
  "description": "A scatterplot showing horsepower and miles per gallons.",
  "data": {"url": "data/cars.json"},
  "mark": "point",
  "encoding": {
    "x": {"field": "Horsepower", "type": "quantitative"},
    "y": {"field": "Miles_per_Gallon", "type": "quantitative"},
    "opacity": {"value": 0.1}
  }
}

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 8, 2016

Fix it. Now value works too.

@kanitw
Copy link
Member

kanitw commented May 8, 2016

Can you add test and example for all these cases?

  • opacity.value
  • opacity.field
  • opacity's stack

kanitw
kanitw reviewed May 9, 2016
View reviewed changes
site/docs/encoding.md
@@ -44,6 +45,7 @@ Mark properties channels map data fields directly to visual properties of the ma
| :------------ |:-------------:| :------------- |
| x, y | [ChannelDef](#def)| X and Y coordinates for `point`, `circle`, `square`, `line`, `text`, and `tick`. (or to width and height for `bar` and `area` marks). |
| color | [ChannelDef](#def)| Color of the marks – either fill or stroke color based on mark type. (By default, fill color for `area`, `bar`, `tick`, `text`, `circle`, and `square` / stroke color for `line` and `point`.) (See [scale range](scale.html#range) for more detail about color palettes.) |
| opacity | [ChannelDef](#def)| Opacity of the marks – either can be a value or in a range. (By default, range of opacity is `[0.3, 0.8]`.) |
Copy link
Member

Choose a reason for hiding this comment

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

Inconsistent default value style.

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 10, 2016

Fix docs. Tests and examples are added.

kanitw
kanitw reviewed May 10, 2016
View reviewed changes
examples/specs/stacked_bar_opacity.json
}
},
"config": {
"mark": {"opacity": 0.6, "stacked" : "none"}
Copy link
Member

Choose a reason for hiding this comment

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

This file is not a stacked bar. Please correct the name.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe bar_layered_opacity?

@kanitw
Copy link
Member

kanitw commented May 10, 2016

Please also update the description about to mention "fix #"

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 10, 2016

Renamed examples

kanitw
kanitw reviewed May 10, 2016
View reviewed changes
examples/specs/stacked_bar_opacity.json
"mark": "bar",
"config": {
Copy link
Member

Choose a reason for hiding this comment

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

This config is unused.

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 10, 2016

Just fix those examples (again).

kanitw
kanitw reviewed May 10, 2016
View reviewed changes
examples/specs/stacked_bar_opacity.json
"type": "quantitative",
"aggregate": "count"
},
"color": {
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 to add color in this example, seems like a distraction.

kanitw
kanitw reviewed May 10, 2016
View reviewed changes
examples/specs/area_opacity_xfield.jason
@@ -0,0 +1,16 @@
{
"data": {"url": "data/unemployment-across-industries.json"},
Copy link
Member

Choose a reason for hiding this comment

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

this file is too large, taking a while to load.

@YuhanLu
Copy link
Contributor Author

YuhanLu commented May 10, 2016

Fixed

kanitw
kanitw reviewed May 10, 2016
View reviewed changes
examples/specs/scatter_opacity_yfield.json
@@ -0,0 +1,9 @@
{
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 two of these?

why not just one scatter_opacity example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I use two to show opacity with different fields (from x and y) and scatter_opacity is using value 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.

I see.

Let's pick one of these scatter_opacity and remove scatter_opacity because our unit test is covering this anyway.

My rationale: the purpose of visual test is so that we can skim the gallery and check if everything is still in good shape. If we add too many redundant examples, there would be too many things to look at anyway.

That said, please search through all existing examples specs. If we use config.mark.opacity, let's use encoding.opacity.value instead so it will cover the same test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Duplicated examples are deleted.
I checked every existed examples with config.mark.opacity and changed them to encoding.opacity.value and they all work well.

@kanitw kanitw merged commit 4739c4f into master May 10, 2016
@kanitw kanitw deleted the zl/channel-opacity2 branch May 10, 2016 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@YuhanLu @kanitw