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

Update for D3 v4 #2246

Merged
merged 27 commits into from
Mar 24, 2018
Merged

Update for D3 v4 #2246

merged 27 commits into from
Mar 24, 2018

Conversation

masayuki0812
Copy link
Member

@masayuki0812 masayuki0812 commented Jan 1, 2018

I just updated c3 to work with d3 v4.

Breaking changes I committed:

  • Disabled drag in zoom when data.selection.draggable = true because of new d3 drag behavior. (zoom and data.selection.draggable still work together, but zoomed section cannot be moved by drag.)
  • Disabled multiple event rects because of new d3 zoom behavior. (This could be avoided but only one event rect is very efficient, so this change seems not bad in my opinion.)
  • Changed internal functions' interface to accept transitions because of new d3 transition behavior. (Transition is not inherited within child selections in v4, so transition object needs to be passed to each draw function.)
  • Renamed axis.x.extent to axis.x.selection based on d3.v4 update.
  • Renamed zoom.extent to zoom.initialRange based on d3.v4 update.
  • Changed api.tooltip.show interface. (Removed index and added id to specify target of data)
  • Changed axis.y.tick.time.value to axis.y.tick.time.type and to receive d3 interval functions (e.g. d3.timeSecond).

#2203 and my changes should be the one later.


closes #2143

@pchaganti
Copy link

👍

@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #2246 into master will increase coverage by 1%.
The diff coverage is 78.47%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #2246    +/-   ##
========================================
+ Coverage   75.82%   76.83%    +1%     
========================================
  Files          51       51            
  Lines        4227     4122   -105     
========================================
- Hits         3205     3167    -38     
+ Misses       1022      955    -67
Impacted Files Coverage Δ
src/config.js 95.83% <ø> (ø) ⬆️
src/type.js 98.36% <ø> (-0.06%) ⬇️
src/data.convert.js 79.41% <0%> (ø) ⬆️
src/api.tooltip.js 16.66% <0%> (+1.66%) ⬆️
src/drag.js 5.76% <0%> (+0.1%) ⬆️
src/data.js 79.62% <0%> (+9.54%) ⬆️
src/api.flow.js 1.29% <0%> (+0.02%) ⬆️
src/scale.js 100% <100%> (ø) ⬆️
src/axis.js 95.96% <100%> (+0.21%) ⬆️
src/color.js 96.77% <100%> (ø) ⬆️
... and 21 more

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 76582fb...233a9c0. Read the comment docs.

@kt3k kt3k self-requested a review January 25, 2018 14:52
@tgaff
Copy link

tgaff commented Feb 14, 2018

Not to nag, but d3 is on RC3 of v5 now. https://github.com/d3/d3/releases

@nsmgr8
Copy link

nsmgr8 commented Feb 22, 2018

d3 v5 doesn't seem to have v4 incompatibility issues.

Very much waiting for this to be merged and released!

@dborstelmann
Copy link

Now using this feature branch in a production project because Salesforce Lockerservice breaks with d3v3. Would love to use the actual release version! Thanks.

@dborstelmann
Copy link

dborstelmann commented Mar 16, 2018

@masayuki0812 We are noticing an error when hovering over bars which creates the tooltips.

[Cannot read property 'getItem' of undefined]
ChartInternal.c3_chart_internal_fn.isWithinBar()

Have you seen this before or do we need to fix it ourselves by hacking it together for the time being?

@wuno
Copy link

wuno commented Mar 18, 2018

@masayuki0812 After we received the initial error in Sales Force,

[Cannot read property 'getItem' of undefined]
ChartInternal.c3_chart_internal_fn.isWithinBar()

We noticed that this problem had to do with SVGPathSegList not being in scope anymore once the component loaded in Sales Force. I realize this was not made with Sales Force lightning's Locker Service in mind but we were able to come up with a solution.

I am wondering what your opinion is on this solution and if you can suggest a way we can make it better or solve our problem all together.

Since the specific function failing was isWithinBar we added these 2 lines to bring SVGPathSegList back in scope.

var seg0 = new window.SVGPathSegList(that).getItem(0);
var seg1 = new window.SVGPathSegList(that).getItem(1);

This is what the function looks like now,

c3_chart_internal_fn.isWithinBar = function (mouse, that) {
    var seg0 = new window.SVGPathSegList(that).getItem(0);
    var seg1 = new window.SVGPathSegList(that).getItem(1);
    var box = that.getBoundingClientRect(),
        x = Math.min(seg0.x, seg1.x),
        y = Math.min(seg0.y, seg1.y),
        w = box.width,
        h = box.height,
        offset = 2,
        sx = x - offset,
        ex = x + w + offset,
        sy = y + h + offset,
        ey = y - offset;

    return sx < mouse[0] && mouse[0] < ex && ey < mouse[1] && mouse[1] < sy;
};

We are confident that this solves the immediate problem of hovers on bar graphs not showing up, but the concern is what else is being affected due to SVGPathSegList not being in scope.

If you can think of a good way for us to solve this throughout your C3.js PR based on the problem and the way we solved the problem, please let us know.
@dborstelmann

@masayuki0812
Copy link
Member Author

@dborstelmann @wuno
Thank you for reporting, but can you open a new issue about SVGPathSegList in SalesForce? I don't think it's not related to this update and I'm not going to fix that in this PR.

@kt3k kt3k merged commit ccff119 into master Mar 24, 2018
@kt3k kt3k deleted the feature/d3_v4 branch March 28, 2018 05:31
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

Successfully merging this pull request may close these issues.

Migrating to d3 v4
8 participants