c3js / c3

:bar_chart: A D3-based reusable chart library

http://c3js.org

MIT License

9.33k stars 1.39k forks source link

Make legend toggle work in IE11 #2741

Closed GDFaber closed 4 years ago

GDFaber commented 4 years ago

Replaced display: initial by display: block in Chart.prototype.show() to stop hidden data items from disappearing in IE11 when they should be shown again (as described in #2528).
Built and tested the changes locally and run tests according to contributing guide.
Did not add new tests; if I should do that here, please give me a pointer on how it is done in this case.

codecov-io commented 4 years ago

Codecov Report

Merging #2741 into master will decrease coverage by 0.02%. The diff coverage is 73.33%.

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
- Coverage   82.85%   82.82%   -0.03%     
==========================================
  Files          60       60              
  Lines        4776     4791      +15     
==========================================
+ Hits         3957     3968      +11     
- Misses        819      823       +4

Impacted Files	Coverage Δ
src/api.show.js	`88.88% <ø> (ø)`	:arrow_up:
src/util.js	`92.42% <73.33%> (-5.62%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2f35319...90087c7. Read the comment docs.

kt3k commented 4 years ago

@GDFaber Thank you for looking into this. The change looks almost good, but we'd like to minimize the effect of change. So can you add the conditional like the below instead of just changing to 'block'?

isIE ? 'block' : 'initial'

GDFaber commented 4 years ago

Hi @kt3k,

sure, I'll add IE detection as stated above. I have a question about that:

I found a check for IE9 in clip.js/ChartInternal.prototype.getClipPath() as well. Should I add a detection function to util.js (as a part of this PR) and let getClipPath() use it (this would be a new PR then)? or is it ok to just check locally and leave clip.js as it is?

kt3k commented 4 years ago

Should I add a detection function to util.js (as a part of this PR) and let getClipPath() use it (this would be a new PR then)?

That sounds good to me.

GDFaber commented 4 years ago

Added functions for IE detection in utils.js
Added basic unit tests for the IE detection functions