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

The heigh of 'Bubble chart' and 'Dual axes, line and column' with Bootstrap #6261

Closed
LuanComputacao opened this Issue Jan 18, 2017 · 14 comments

Comments

Projects
None yet
7 participants
@LuanComputacao

LuanComputacao commented Jan 18, 2017

Expected behaviour

Feel the size of recipient!

Actual behaviour

Resizing to 1px

Live demo with steps to reproduce

Affected browser(s)

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Jan 19, 2017

Collaborator

Can you provide a live demo please? It works fine here: http://jsfiddle.net/highcharts/pxd6js0x/

It may be related to #6217. Prior to v5.0.7, the chart would size up to 400px height if the container height was less than 19px.

Collaborator

TorsteinHonsi commented Jan 19, 2017

Can you provide a live demo please? It works fine here: http://jsfiddle.net/highcharts/pxd6js0x/

It may be related to #6217. Prior to v5.0.7, the chart would size up to 400px height if the container height was less than 19px.

@DoomHawk

This comment has been minimized.

Show comment
Hide comment
@DoomHawk

DoomHawk Jun 13, 2017

After years of feeling like a leech off all the support forums I've used to fix problems like this, I finally get to be part of the solution! I have been able to replicate it in at least one scenario, where it seems that at best there is a direct issue regarding Bootstrap grid css and this Highcharts bug; or at worst, css being applied directly to the chart's render div is causing the bug. Anyway, that's my two cents, here's the Fiddle:

http://jsfiddle.net/robert_ivaniszyn/p7rL7yfz/1/

Hope this helps!

Here's a screenshot of the inspected div showing that it has been given an element styling height of 1px.

As an additional test, and potential workaround for current sufferers: If you remove the Bootstrap grid css from the #TrendChart div, the chart will render at full height of the parent.

DoomHawk commented Jun 13, 2017

After years of feeling like a leech off all the support forums I've used to fix problems like this, I finally get to be part of the solution! I have been able to replicate it in at least one scenario, where it seems that at best there is a direct issue regarding Bootstrap grid css and this Highcharts bug; or at worst, css being applied directly to the chart's render div is causing the bug. Anyway, that's my two cents, here's the Fiddle:

http://jsfiddle.net/robert_ivaniszyn/p7rL7yfz/1/

Hope this helps!

Here's a screenshot of the inspected div showing that it has been given an element styling height of 1px.

As an additional test, and potential workaround for current sufferers: If you remove the Bootstrap grid css from the #TrendChart div, the chart will render at full height of the parent.

@pawelfus

This comment has been minimized.

Show comment
Hide comment
@pawelfus

pawelfus Jun 13, 2017

Contributor

Thanks for sharing!

It's caused by CSS in Bootstrap:

.col-lg-1, .col-lg-10, .col-lg-11, .col-lg-12, .col-lg-2, .col-lg-3, .col-lg-4, .col-lg-5, .col-lg-6, .col-lg-7, .col-lg-8, .col-lg-9, .col-md-1, .col-md-10, .col-md-11, .col-md-12, .col-md-2, .col-md-3, .col-md-4, .col-md-5, .col-md-6, .col-md-7, .col-md-8, .col-md-9, .col-sm-1, .col-sm-10, .col-sm-11, .col-sm-12, .col-sm-2, .col-sm-3, .col-sm-4, .col-sm-5, .col-sm-6, .col-sm-7, .col-sm-8, .col-sm-9, .col-xs-1, .col-xs-10, .col-xs-11, .col-xs-12, .col-xs-2, .col-xs-3, .col-xs-4, .col-xs-5, .col-xs-6, .col-xs-7, .col-xs-8, .col-xs-9 {
    position: relative;
    min-height: 1px; // here is the problem
    padding-right: 15px;
    padding-left: 15px;
}

To explain: http://jsfiddle.net/p7rL7yfz/3/ - height of the container is 1px, so this is what Highcharts use to render a chart.

Another workaround: http://jsfiddle.net/p7rL7yfz/4/ - set directly min-height: auto for the container.

Related issue on Bootstrap: twbs/bootstrap#13640

Contributor

pawelfus commented Jun 13, 2017

Thanks for sharing!

It's caused by CSS in Bootstrap:

.col-lg-1, .col-lg-10, .col-lg-11, .col-lg-12, .col-lg-2, .col-lg-3, .col-lg-4, .col-lg-5, .col-lg-6, .col-lg-7, .col-lg-8, .col-lg-9, .col-md-1, .col-md-10, .col-md-11, .col-md-12, .col-md-2, .col-md-3, .col-md-4, .col-md-5, .col-md-6, .col-md-7, .col-md-8, .col-md-9, .col-sm-1, .col-sm-10, .col-sm-11, .col-sm-12, .col-sm-2, .col-sm-3, .col-sm-4, .col-sm-5, .col-sm-6, .col-sm-7, .col-sm-8, .col-sm-9, .col-xs-1, .col-xs-10, .col-xs-11, .col-xs-12, .col-xs-2, .col-xs-3, .col-xs-4, .col-xs-5, .col-xs-6, .col-xs-7, .col-xs-8, .col-xs-9 {
    position: relative;
    min-height: 1px; // here is the problem
    padding-right: 15px;
    padding-left: 15px;
}

To explain: http://jsfiddle.net/p7rL7yfz/3/ - height of the container is 1px, so this is what Highcharts use to render a chart.

Another workaround: http://jsfiddle.net/p7rL7yfz/4/ - set directly min-height: auto for the container.

Related issue on Bootstrap: twbs/bootstrap#13640

@PopRe

This comment has been minimized.

Show comment
Hide comment
@PopRe

PopRe Jul 20, 2017

This issue is still present - there doesn't appear to have been any code change and the workaround needs to be done in the highcharts code

PopRe commented Jul 20, 2017

This issue is still present - there doesn't appear to have been any code change and the workaround needs to be done in the highcharts code

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jul 21, 2017

Contributor

@PopRe The problem seems to be caused by CSS in Bootstrap. Could you provide a demo showing the problem with Highcharts while not using Bootstrap CSS?

Contributor

KacperMadej commented Jul 21, 2017

@PopRe The problem seems to be caused by CSS in Bootstrap. Could you provide a demo showing the problem with Highcharts while not using Bootstrap CSS?

@PopRe

This comment has been minimized.

Show comment
Hide comment
@PopRe

PopRe Jul 21, 2017

It appears to be a slightly different issue:
https://jsfiddle.net/p7rL7yfz/11/

Basically the code at fault is:
return Math.min(el.offsetHeight, el.scrollHeight) - H.getStyle(el, 'padding-top') - H.getStyle(el, 'padding-bottom');
and
chart.chartHeight = Math.max( 0, H.relativeLength( heightOption, chart.chartWidth ) || chart.containerHeight || 400

If the containing element has any padding on the top or bottom, the first will return a negative or small number. The second block will use that (or 0 if it's negative) instead of 400 because chart.containerHeight is not 0

PopRe commented Jul 21, 2017

It appears to be a slightly different issue:
https://jsfiddle.net/p7rL7yfz/11/

Basically the code at fault is:
return Math.min(el.offsetHeight, el.scrollHeight) - H.getStyle(el, 'padding-top') - H.getStyle(el, 'padding-bottom');
and
chart.chartHeight = Math.max( 0, H.relativeLength( heightOption, chart.chartWidth ) || chart.containerHeight || 400

If the containing element has any padding on the top or bottom, the first will return a negative or small number. The second block will use that (or 0 if it's negative) instead of 400 because chart.containerHeight is not 0

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jul 23, 2017

Contributor

@PopRe In your case should the chart have 0px height? If you define height for the chart's container as 100%, then the container height is properly displayed in the HTML and the chart is using it. Demo: https://jsfiddle.net/p7rL7yfz/12/

Contributor

KacperMadej commented Jul 23, 2017

@PopRe In your case should the chart have 0px height? If you define height for the chart's container as 100%, then the container height is properly displayed in the HTML and the chart is using it. Demo: https://jsfiddle.net/p7rL7yfz/12/

@PopRe

This comment has been minimized.

Show comment
Hide comment
@PopRe

PopRe Jul 23, 2017

The height is undefined in my case. Setting it to something like 400px is a work around, however the bug (which is a regression) should still be fixed.

PopRe commented Jul 23, 2017

The height is undefined in my case. Setting it to something like 400px is a work around, however the bug (which is a regression) should still be fixed.

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jul 24, 2017

Contributor

@PopRe "The height is undefined in my case." and in that case default value is 400. If you want something different that you could set it using current API. If you are not satisfied with the current design, please specify how should the algorithm be improved and why.

"the bug (which is a regression) should still be fixed" - Please let us know which version is working correctly for you and please provide a live demo.

Contributor

KacperMadej commented Jul 24, 2017

@PopRe "The height is undefined in my case." and in that case default value is 400. If you want something different that you could set it using current API. If you are not satisfied with the current design, please specify how should the algorithm be improved and why.

"the bug (which is a regression) should still be fixed" - Please let us know which version is working correctly for you and please provide a live demo.

@PopRe

This comment has been minimized.

Show comment
Hide comment
@PopRe

PopRe Jul 24, 2017

What should be fixed is the following:
chart.chartHeight = Math.max( 0, H.relativeLength( heightOption, chart.chartWidth ) || chart.containerHeight || 400

It's suppose to default to 400, however, it instead defaults to a small number such as 0 (if the containerHeight is < 0) or 1 (if the offsetHeight - padding is 1).

A possible fix would be a conditional checking if it is below 5 then set it to the default of 400, or to fix the code calculating the containerHeight

PopRe commented Jul 24, 2017

What should be fixed is the following:
chart.chartHeight = Math.max( 0, H.relativeLength( heightOption, chart.chartWidth ) || chart.containerHeight || 400

It's suppose to default to 400, however, it instead defaults to a small number such as 0 (if the containerHeight is < 0) or 1 (if the offsetHeight - padding is 1).

A possible fix would be a conditional checking if it is below 5 then set it to the default of 400, or to fix the code calculating the containerHeight

@KacperMadej

This comment has been minimized.

Show comment
Hide comment
@KacperMadej

KacperMadej Jul 24, 2017

Contributor

@PopRe Thank you for explaining some of the questions.

  1. "the bug (which is a regression) should still be fixed" - Please let us know which version is working correctly for you and please provide a live demo.

  2. "It's suppose to default to 400, however, it instead defaults to a small number such as 0 (if the containerHeight is < 0) or 1 (if the offsetHeight - padding is 1)." - Please provide a demo and let us know which browser and OS should be used. The last demo you have provided produces a chart with default height of 400px.

  3. If you would like us to fix this as a bug, then it should be first recognized as a bug. Until this happens I could suggest you writing a wrapper as documented here: https://www.highcharts.com/docs/extending-highcharts/extending-highcharts and in case of any problems you can contact us through other support channels: https://www.highcharts.com/support because we are trying to keep github for bugs.

Contributor

KacperMadej commented Jul 24, 2017

@PopRe Thank you for explaining some of the questions.

  1. "the bug (which is a regression) should still be fixed" - Please let us know which version is working correctly for you and please provide a live demo.

  2. "It's suppose to default to 400, however, it instead defaults to a small number such as 0 (if the containerHeight is < 0) or 1 (if the offsetHeight - padding is 1)." - Please provide a demo and let us know which browser and OS should be used. The last demo you have provided produces a chart with default height of 400px.

  3. If you would like us to fix this as a bug, then it should be first recognized as a bug. Until this happens I could suggest you writing a wrapper as documented here: https://www.highcharts.com/docs/extending-highcharts/extending-highcharts and in case of any problems you can contact us through other support channels: https://www.highcharts.com/support because we are trying to keep github for bugs.

@PopRe

This comment has been minimized.

Show comment
Hide comment
@PopRe

PopRe Jul 24, 2017

I've submitted a new issue with the relevant details: #7000

PopRe commented Jul 24, 2017

I've submitted a new issue with the relevant details: #7000

@terryaney

This comment has been minimized.

Show comment
Hide comment
@terryaney

terryaney Sep 7, 2017

It was asked multiple times for 'last working version' and also stated 'problem with bootstrap'...but the fact that 5.0.7 was stated as the version that changed the behavior of how chart heights were determined, can't the code that changed in highcharts be identified versus stating to do a work around in markup (i.e. if we have 10s/100s of clients with markup that worked and now breaks when moving to 5.07+)? Guess I'm just disappointed that this is being closed when the issue still seems very prevalent and I either have to have a fork of highcharts source or figure out how to update hundreds of sites (all multitenant sharing same js references) to work around the problem.

terryaney commented Sep 7, 2017

It was asked multiple times for 'last working version' and also stated 'problem with bootstrap'...but the fact that 5.0.7 was stated as the version that changed the behavior of how chart heights were determined, can't the code that changed in highcharts be identified versus stating to do a work around in markup (i.e. if we have 10s/100s of clients with markup that worked and now breaks when moving to 5.07+)? Guess I'm just disappointed that this is being closed when the issue still seems very prevalent and I either have to have a fork of highcharts source or figure out how to update hundreds of sites (all multitenant sharing same js references) to work around the problem.

TorsteinHonsi added a commit that referenced this issue Sep 11, 2017

@TorsteinHonsi

This comment has been minimized.

Show comment
Hide comment
@TorsteinHonsi

TorsteinHonsi Sep 11, 2017

Collaborator

This regression was caused by the fix to #6217, where a user reported that container sizes below 20px weren't respected. You can read the comments there. So while it makes sense to make 1px containers cause 400px charts, users expect 18px containers to cause 18px charts. The question is where to draw the line. I have committed a fix now that makes the chart draw at 400px height if the container is 1px or less in height. It takes care of the Bootstrap issue, I hope it takes care of other cases too. If not, we can consider raising the limit to for example 5px, but that feels more unpredictable.

Collaborator

TorsteinHonsi commented Sep 11, 2017

This regression was caused by the fix to #6217, where a user reported that container sizes below 20px weren't respected. You can read the comments there. So while it makes sense to make 1px containers cause 400px charts, users expect 18px containers to cause 18px charts. The question is where to draw the line. I have committed a fix now that makes the chart draw at 400px height if the container is 1px or less in height. It takes care of the Bootstrap issue, I hope it takes care of other cases too. If not, we can consider raising the limit to for example 5px, but that feels more unpredictable.

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