-
Notifications
You must be signed in to change notification settings - Fork 250
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
Scale.alpha_continuous and Scale.alpha_discrete #1252
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1252 +/- ##
=========================================
- Coverage 85.74% 85.5% -0.25%
=========================================
Files 35 35
Lines 4119 4139 +20
=========================================
+ Hits 3532 3539 +7
- Misses 587 600 +13
Continue to review full report at Codecov.
|
looks great! any chance you have time to add |
Here's a good example, where there are lots of bunched points: D = dataset("datasets","faithful")
D[:g] = D[:Eruptions].>3.0
ylab = Guide.ylabel(nothing)
p1 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
Theme(discrete_highlight_color=c->nothing, alphas=[0.4], key_position=:inside) )
p2 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
Theme(discrete_highlight_color=c->"white", key_position=:inside) )
p3 = plot(D, x=:Eruptions, y=:Waiting, color=:g, Geom.point, ylab,
Theme(discrete_highlight_color=identity, alphas=[0.4], key_position=:inside) )
hstack(p1, p2, p3) |
so in the regression tests, the points in the plot are the same as before, it's just that the swatches are different? if so, and it's now the case that the swatches are identical to the points in the plot (except for the mismatch in shapes), whereas before they weren't, then i'd consider this a bug fix, not a breaking change. IIUC, any changes to |
Yes the points in the plot are the same as before, the swatches (in discrete colorkeys) are smaller because they get stroked with white (the I'll do |
I noticed that if you click on the graphics in my 2 previous posts, the white stroke around the key swatches in the middle plot can clearly be seen (I'm using Chrome). If there are no other comments, this PR is ready to merge. |
@@ -177,14 +177,14 @@ Gadfly.helpscreen_visible = function(event) { | |||
helpscreen_visible(this.plotroot()); | |||
}; | |||
var helpscreen_visible = function(root) { | |||
root.select(".helpscreen").animate({opacity: 1.0}, 250); | |||
root.select(".helpscreen").animate({"fill-opacity": 1.0}, 250); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this appears to be a duplicate of c62ef81, and is how the code already looks on master. how is it then that this is showing up as a change in the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point I rebased this branch on master, because I started it before c62ef81. What does github think the diff is relative to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you click on the "commits" tab above it says "bjarthur authored and Mattriks committed 12 days ago". i think the rebase messed things up. am worried master will get messed up if i merge this. can you please see if you can fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did you do your rebase? my workflow is as follows:
git checkout master
git pull
git checkout -b newPR
git add
git commit
git checkout master
git pull
git checkout newPR
git rebase master
the key here is that master is rebase onto newPR, not vice versa, which is what i think you might have done.
@@ -213,7 +214,8 @@ hstack(p3, p4) | |||
| `shape` | `shape_discrete` | `shapekey` | | |||
| `size` | `size_discrete` | sizekey (tbd) | | |||
| `linestyle` | `linestyle_discrete` | linekey (tbd) | | |||
| `group` | `group_discrete` | | | |||
| `alpha` | `alpha_discrete` | alphakey (tbd) | | |||
| `group` | `group_discrete` | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure the tables of Scales are appropriate in the Tutorial. they are all enumerated in the Library. we don't enumerate all Guides or Geoms. and listing them here disrupts the flow of teaching by example. i might submit a PR to smooth things out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tables were introduced in a previous PR (#1245): http://gadflyjl.org/dev/tutorial/. The relationship between aesthetic, scales and guides is a generally not well understood, and hence these tables need to be in the tutorial.
Closed in favor of #1257 |
NEWS.md
This PR:
Scale.alpha_continuous()
andScale.alpha_discrete()
Theme(alphas=[])
So it is no longer necessary to use
Theme(lowlight_color=)
for opacity. But for exampleTheme(lowlight_color=c->"gray")
is still useful for changing the line color ofGeom.ribbon
andGeom.polygon
independently of thecolor
aesthetic.Examples: