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

Brush rect is flat #1134

Closed
gordonwoodhull opened this issue May 3, 2016 · 6 comments
Closed

Brush rect is flat #1134

gordonwoodhull opened this issue May 3, 2016 · 6 comments
Labels
Milestone

Comments

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented May 3, 2016

@paulbriton reported on gitter:

I'm having trouble with the brush and the include of dc.js with RequireJs.. When I include the libs with the classic <script> every thing is ok but when it's done with requirejs brush background isn't showed. After further investigations it seems that the height property is not set on the elements when using requirejs. There is no js errors, and when I set manually the height of both element it's ok. Any explantions ?

Bug

Bug is present since beta 16.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone May 3, 2016
@paulbriton
Copy link

Here is two commits between beta 15 and 16 about the brush :

I will try later this week to give a fiddle demonstrating the problem.

By the way, thank's for your availability.

@paulbriton
Copy link

I've tried to reproduce it in a fiddle but I didn't manage to have the same error.

I know that without a reproductible case, it's hard to correct, but the error is still present in the latest releases.

@gordonwoodhull
Copy link
Contributor Author

The commits between those releases are mostly to do with resizing transitions. Not that it's a solution, but does the brush show up if you set transitionDuration(0)?

Other than that, if you send me a link to my private email (on my github profile) I can debug it confidentially. It takes a lot of time just guessing but it should be easy to track down in the debugger.

@paulbriton
Copy link

The transitionDuration(0) doesn't show the brush.

I know that giving you the access should be easier but it's not my call since we have confidential information on this site. I will ask and let you know.

As always, thank's for your availability.

@gordonwoodhull
Copy link
Contributor Author

gordonwoodhull commented Jun 29, 2016

@paulbriton tracked it down to this line in coordinateGridMixin.setBrushY:

        gBrush.selectAll('.brush rect')
            .attr('height', brushHeight());

gBrush.selectAll('.brush rect')

which used to selectAll('rect') but was changed in 354b890 when the intent was just to change the brush handle size. Looks like an unintentional change.

What's curious is that when I try this in the debugger, it still selects all four child rects, even though selection.selectAll is documented as selecting only descendants.

So I think the code is incorrect but may work with some versions of d3 but not others (?) I can't explain otherwise how the use of require would affect this code.

gordonwoodhull added a commit that referenced this issue Jun 29, 2016
gordonwoodhull added a commit that referenced this issue Jun 29, 2016
that often worked
thanks @paulbriton for tracking this down!
this seems to be a mistake introduced in 354b890
fixes #1134
gordonwoodhull added a commit that referenced this issue Jun 29, 2016
gordonwoodhull added a commit that referenced this issue Jun 29, 2016
gordonwoodhull added a commit that referenced this issue Jun 29, 2016
that often worked
thanks @paulbriton for tracking this down!
this seems to be a mistake introduced in 354b890
fixes #1134
@gordonwoodhull
Copy link
Contributor Author

Thanks @paulbriton, released this in 2.0 beta 31

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

No branches or pull requests

2 participants