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

chart height changes by using the brush #980

Closed
christianmalek opened this issue Aug 11, 2015 · 17 comments
Closed

chart height changes by using the brush #980

christianmalek opened this issue Aug 11, 2015 · 17 comments
Milestone

Comments

@christianmalek
Copy link

See here:
Imgur

This problem occurs in beta15, beta16 and beta17. beta 14 works fine for me.
It's also only on the stacked bar charts. The line chart doesn't have this problem.

@gordonwoodhull
Copy link
Contributor

It has to be an interaction with the resize logic in your own code, since dc does not resize divs. Nonetheless, we will figure it out.

Typically the axis will land in the wrong place if it's on a transition and then another modification comes along. Do you already have code which is trying to do the same thing as the fixes in beta 15, move the axis to fit the div?

@christianmalek
Copy link
Author

I only change the divs which contain the chart svgs. If I need to resize the charts I use the .width(...) and .height(...) methods from dc.

@mueslo
Copy link

mueslo commented Oct 27, 2015

I have the same issue, any updates? I have no code that changes sizes either. The issue did not occur on 1.7, but does occur on 2.0b19. Height increases by 5 on every brush usage.

Here's a stack trace of what's modifying the svg height, if it's of help. Although both seem to be resized, it's just that the height changes while the width stays constant.
stack trace
stack trace

@mueslo
Copy link

mueslo commented Oct 27, 2015

Ah, it seems that the problem lies in the fact that the root

seems to always be 5 larger than the in my case. And _chart.height resizes based on the root node size, if no value is given. However, this is also the case for another chart, where this does not happen. On the one where it happens I did not explicitly set the chart height. So just setting the chart height fixed it for me.

@gordonwoodhull
Copy link
Contributor

Thanks @mueslo. So, are you setting chart.height() based on the root div size? I don't think dc.js will look at the height of its container, right?

@mueslo
Copy link

mueslo commented Oct 28, 2015

Before I didn't set any chart height for the affected chart, now I'm just setting a fixed value. If no value is given, it just sets itself to the height of the element it is anchored to, see

dc.js/src/base-mixin.js

Lines 130 to 135 in cd61554

_chart.height = function (height) {
if (!arguments.length) {
return _height(_root.node());
}
_height = d3.functor(height || _defaultHeight);
return _chart;

@gordonwoodhull
Copy link
Contributor

Ah, thank you @mueslo for reminding me of that other piece of responsive behavior. Yes, currently if you are not setting the width and height, then they have to be consistent with the width and height of the anchor/container element.

Which makes me wonder how useful the automatic width/height are, since there is almost always a filter display or a title within the anchor div, taking part of the space. Perhaps these margins or adjustments have to be added to the calculation.

@gordonwoodhull
Copy link
Contributor

@Phisherman, does this also explain the behavior you're seeing?

@christianmalek
Copy link
Author

@gordonwoodhull: I now changed to beta19 and set the height of the bar chart to an absolute value of 200. Still the same behaviour. Every time i change the brush selection the bars move a bit under the x axis.

But if I set a fixed height for the div container which embeds the bar chart it works (setting chart.height(x) will be ignored in this case).

Why does this behaviour only apply to bar charts? With line charts there isn't this problem. I liked the behaviour of beta14 way more. The chart's height should be set with dc.js.

@gordonwoodhull
Copy link
Contributor

Thanks @Phisherman. It's an important clue that the problem occurs when the containing div in your app does not have a fixed height. Is it using a percentage?

It is very strange that chart.height() should return anything other than a constant if you set it with a constant. I can't find an explanation for that in the code, which sets the _height function to a d3.functor which simply returns the value.

If you are able to create a fiddle, or share a link with me (publicly or by email), we can track this down better.

@christianmalek
Copy link
Author

@gordonwoodhull Nope. I used a fixed px definition. At the moment I can't create a fiddle, sorry.

@gordonwoodhull
Copy link
Contributor

I'm not sure what to do about this, since

  1. Setting the SVG size based on the container size on render is obviously useful.
  2. Setting the SVG size from width and height on redraw is also useful.
  3. The container size is often larger than the SVG size after rendering. E.g. when controls appear, which must be part of the container. I assume padding / margins can also screw this up.

@MatthiasJobst
Copy link

Not sure that I agree @gordonwoodhull on your point 1. In order to have better control over layout and where controls go I tend to have a structure like bootstrap panel. In this case the actual container of the graph might not have the correct size. As you state in 3 it also changes with availability of content.
Not sure if that is a useful comment, but I think that 1 only works in trivial cases.
What puzzles me with the problem in #1040 is that the axis remain in the original width only the bars change size.

@gordonwoodhull
Copy link
Contributor

Thanks @MatthiasJobst, this is a good data point. Maybe I should have said setting the size on render is "sometimes useful". It definitely works for some people, but it breaks down in very simple cases like the fiddle you demonstrated.

The reason that the content changes size while the axes don't, is that you currently need to call rescale in order to get a size change to register. So the supported way to resize a chart is:

chart
    .width(...)
    .height(...)
    .rescale();

But the size is applied regardless whether or not you call this, so they get out of sync. Two things which are not great - we are here due to retrofitting resize behavior onto a library that didn't originally support it.

As noted above, somewhere the interface has to be broken - something we can consider for v2.1. It might be as simple as always applying the rescale logic when redrawing, and deprecating rescale. However, that still does not answer how the size should be applied when it's not specified. I suppose we could make width and height required again, but this will break a lot of code.

@MatthiasJobst
Copy link

@gordonwoodhull, thanks for the detailed explanation of the problem. For the use cases that I have experience with rescaling was no issue. From that point of view deprecating rescale would be ok.

Is there a difference between rescale, redraw or render? In another not requested point of view of mine the dynamic behavior of the size is a feature that should be handled outside of dc. With that I mean that the resizing of a graph probably has to do with some logic of the application, but not of the graph. So it should be handled by the application. Especially if there is no performance penalty of calling redraw this is no disadvantage but a separation of concerns.

Regarding the size question I think that width and size should be set on the initial render, but then used as constant unless explicit function calls change them like you describe in your post.

@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Dec 14, 2015
@gordonwoodhull
Copy link
Contributor

@MatthiasJobst, rescale just marks the chart as needing to recalculate things to do with a size change - it doesn't perform an action in itself. (The name is a little misleading.) I think we should just fold that into redraw in 2.1 and figure out if there were any places it causes too much change - if it was only added to avoid performance problems then it was probably a mistake.

I agree that the chart shouldn't be dynamically sizing itself - I don't think that was ever intended. What's going on here is that one way or another the div size is based on the svg and the svg is also based on the div, causing recursively annoyance. I've verified that if the div width and height are absolute pixel values, then the bug also doesn't manifest.

I also found at least one other related bug that already existed before we started sizing the SVG on redraw: #1070

I agree the solution is to only auto-calculate the size once.

@gordonwoodhull
Copy link
Contributor

Correction: we should auto-calculate the size on each render, even if this can cause the height change problems described here, because it is expected that everything gets recalculated on render. And this was the behavior before beta 15.

However, we should only calculate once, to avert the glitches described in #1070.

Fixed in 2.0 beta 24.

gordonwoodhull added a commit that referenced this issue Dec 21, 2015
we don't know the actual sizes here
the main point is the failing redraw test for #980/#1070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants