-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Add stroke attribute to line-chart dots #1447
Comments
I agree, that's odd. I can only guess that the person who contributed Are you sure that setting stroke as an attribute helps? Deepening the mystery, we have .dc-chart circle.dot {
stroke: none
} And CSS trumps attributes, so changing the attribute on the circle element doesn't do anything in my testing. I'm definitely willing to fix this, but I'd like to leave the default with no stroke somehow. Hmmm, kind of a mess. |
Ah, yea. I forgot I had removed that CSS rule in a local override. Looking through the code, I can't find any other place where the stroke is being set. Maybe removing that CSS rule and setting the default stroke-opacity to 0? |
Yes, that sounds backward compatible since the stroke opacity didn't have any effect without changing the CSS. Would you have any interest in filing a PR for this? |
Yep, I'm assuming this would not count as an API change so I should branch off master right? |
Would it be possible to include this fix with v2? If so how would I go about doing that? I've tested the changes with our local copy of v2 but we want to avoid making local changes. |
Sure, I'm supporting 2.* for a while, while people migrate to dc v3 and D3v4. I can do another 2.2.* release. Technically, you could base your PR on the |
Fixed in 2.2.1 & 3.0.6 |
For line-charts we are given the following options for renderDataPoints:
{fillOpacity, strokeOpacity, radius}
but strokeOpacity is unused since the rendered dots don't have a stroke attribute defined. Would it be possible to add that attribute? Adding:
.attr('stroke', _chart.getColor)
to dc.line-chart.drawDots() on line 394 seems to have the desired effect. Or is there a reason why that attribute is excluded?
The text was updated successfully, but these errors were encountered: