-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Implement fgopacity, fgcolor & "overlay" fillmode for bars, fix bar legend and pattern with marker colorscale #5733
Conversation
- also fixing missing bar legend color issue 5285
Behaviour looks pretty good at first glance! Will dig more tomorrow but nice job :) |
The legend items get hard to identify when the |
I'm thinking that we need to tune things a little bit because for the same size/solidity, the different patterns feel quite different to my eye. Also I'm thinking we need to either make the overlay semi-transparent or add |
Well, playing with it a bit I can see this will be a rabbit-hole so let's leave this alone for now :) so my asks are:
|
What about |
sounds good. |
Good call. The scale for all shapes of legends is now reduced by fd89e23. |
OK, this behaviour is all good for me 🌟 @alexcjohnson just need a dancer :) |
src/components/drawing/index.js
Outdated
@@ -500,7 +529,8 @@ drawing.pattern = function(sel, gd, patternID, shape, bgcolor, fgcolor, size, so | |||
'id': fullID, | |||
'width': width + 'px', | |||
'height': height + 'px', | |||
'patternUnits': 'userSpaceOnUse' | |||
'patternUnits': 'userSpaceOnUse', | |||
'patternTransform': isLegend ? 'scale(0.7)' : '' |
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.
Can we adjust the size from the legend side, rather than detecting the legend inside here? Also do we need to scale down all these sizes? I'd rather we just restrict the maximum size to something we know will work, and perhaps when we have an array for pattern size just use the default size instead of the first one.
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.
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.
Again though, do we need to scale down to 0.7 here? In the legend style we already limit the size to 12, can't we keep the same scale as on the graph, but if 12 is too big just reduce that limit?
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.
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 think the current legend items are "ok" ... better than on master, and we can always tweak them a bit more later. I really do need to get this merged and released so unless there's a hard blocker/pathological case here, can we defer the remaining work @alexcjohnson ?
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.
OK
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 image test does not show a green light this branch at the moment. |
Resolves #5728 and fixes #5285.
Also fixes drawing bar
pattern
when the marker get acolorscale
.@plotly/plotly_js