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

Transition line-chart dots along with the lines #1181

Closed
wants to merge 1 commit into from
Closed

Transition line-chart dots along with the lines #1181

wants to merge 1 commit into from

Conversation

paulmach
Copy link
Contributor

@paulmach paulmach commented Jul 9, 2016

When updating data for line-charts, the line updates using a d3.transition(). The dots (if enabled) do not use a transition. This PR adds transitions to dots. See the updated series example.

I'm not exactly sure what test to write. Maybe check location, d3.timer.flush(), check location is different?
Also, probably should not update the example, but leaving in for now as a demo for anyone reviewing this.

@gordonwoodhull
Copy link
Contributor

Thanks @paulmach! I've started some work in this area in a branch, feature/area-transitions, and this will be helpful to compare and merge.

Also your changes are interface-compatible, whereas that branch starts to tackle the big problem of joining by key and transitioning stacks in and out. So it might be good to apply your changes first in 2.0.

As for tests, I don't think automated testing is feasible for transitions. So I'm starting to create as many "eyeball tests" as I can, here. I guess that's similar to what you've done with the series example.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Jul 9, 2016
@paulmach
Copy link
Contributor Author

paulmach commented Jul 9, 2016

Thanks for the response. I would like to apply this to 2.0. It would nice to make use of this update for a project at work.

The "stacks" area transition example illustrates the problem. The dots move, while the lines transition. So I think that's a good visual test.

I just removed the change to the stacks example as that was just for me before you pointed me to the area transitions example/test.

@@ -288,6 +288,7 @@ dc.lineChart = function (parent, chartGroup) {
.attr('r', getDotRadius())
.style('fill-opacity', _dataPointFillOpacity)
.style('stroke-opacity', _dataPointStrokeOpacity)
.attr('fill', _chart.getColor)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, without this the dots will be black and transition to the color. I caught this visually but this spec will also catch it.

gordonwoodhull added a commit that referenced this pull request Jul 27, 2016
and revert flushing of transitions, no longer necessary here
for #1181
gordonwoodhull added a commit that referenced this pull request Jul 28, 2016
and revert flushing of transitions, no longer necessary here
for #1181
@gordonwoodhull
Copy link
Contributor

Rebased and merged in 2.0 beta 32. Thanks @paulmach!

@paulmach
Copy link
Contributor Author

paulmach commented Aug 4, 2016

Thanks @gordonwoodhull

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants