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

Add zoom type 'drag' option (based on #1500) #2439

Merged
merged 7 commits into from Aug 7, 2018
Merged

Add zoom type 'drag' option (based on #1500) #2439

merged 7 commits into from Aug 7, 2018

Conversation

@kt3k
Copy link
Member

@kt3k kt3k commented Aug 3, 2018

This PR is a continuation of #1500. #1500 is not mergeable because of d3's breaking changes and c3's refactoring. I did the following to fit the feature to the current code base.

  • Rebased #1500
  • Updated d3 API calls to follow the breaking changes between v3 and v5 (and did some refactoring)
  • removed disableDefaultBehavior config because I couldn't find the use case of zoom.disableDefaultBahavior: true

The preview of this change is available here (circleci artifact)

@mistic
Could you check if the above page's behavior reflects your original idea of the feature?

@mistic
Copy link
Contributor

@mistic mistic commented Aug 7, 2018

@kt3k the code looks okay and the features are there. I think we just should include the disableDefaultBehavior behaviour again (you can think on another name if you found this one a bit difficult to grasp the real meaning). I found this feature very useful for the case where we have aggregated data, and so the drag zoom will only return the x-domain, we re-quest the new data and then reload the chart with the new data.

mistic and others added 5 commits Dec 4, 2015
…default zoom behavior.

Added uncorrect removed files.

Fixed build files permissions.

Fixed visibility property spelling.

Fixed bug on returned zoom domain in 'onzoomend' event.

Prevent a drag from a single click.

removed fix for false click.

Fixed bug in update zoom when zoom type is drag.
@kt3k kt3k force-pushed the feature/zoom-type branch from e7eae91 to 38e3945 Aug 7, 2018
@codecov-io
Copy link

@codecov-io codecov-io commented Aug 7, 2018

Codecov Report

Merging #2439 into master will decrease coverage by 0.27%.
The diff coverage is 48.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2439      +/-   ##
==========================================
- Coverage   80.75%   80.48%   -0.28%     
==========================================
  Files          54       54              
  Lines        4246     4279      +33     
==========================================
+ Hits         3429     3444      +15     
- Misses        817      835      +18
Impacted Files Coverage Δ
src/config.js 95.83% <ø> (ø) ⬆️
src/core.js 91.21% <100%> (+0.02%) ⬆️
src/zoom.js 70.12% <45.45%> (-19.01%) ⬇️

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 e3c895b...bce7489. Read the comment docs.

@kt3k
Copy link
Member Author

@kt3k kt3k commented Aug 7, 2018

@mistic I restored zoom.disableDefaultBehavior option. Here is the example of disableDefaultBehavior: true on both scroll and drag types.

@kt3k kt3k closed this Aug 7, 2018
@mistic
Copy link
Contributor

@mistic mistic commented Aug 7, 2018

@kt3k perfect! I think it's ready to merge whenever you want 😄

@kt3k kt3k reopened this Aug 7, 2018
@kt3k
Copy link
Member Author

@kt3k kt3k commented Aug 7, 2018

@mistic Thanks for confirming! 🙏
I appreciate your patience. Thank you again for the original idea and initial implementation!

@kt3k kt3k merged commit 06df83e into master Aug 7, 2018
3 checks passed
3 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
circleci/htdocs preview build succes!
Details
@kt3k kt3k deleted the feature/zoom-type branch Aug 7, 2018
@mistic
Copy link
Contributor

@mistic mistic commented Aug 7, 2018

No problem @kt3k :) I believe this will add value to c3js! Thanks for the effort on this.

@kt3k kt3k mentioned this pull request Aug 8, 2018
@mageshveeraa
Copy link

@mageshveeraa mageshveeraa commented Aug 9, 2018

@kt3k @mistic If I set zoom type as 'drag', tooltip not showing..

And, another problem I am facing is: Every time I zoom the chart zoom out to orginal state and zooming... I need multi level zoom... for example... First I zoom 1 to 2pm.. then I want to 1:45 to 1:55pm...

@EliasC10
Copy link

@EliasC10 EliasC10 commented Sep 20, 2018

@kt3k @mistic If I set zoom type as 'drag', tooltip not showing..

@mageshveeraa i have the same problem, did you find a solution for that?

@Federico-G
Copy link

@Federico-G Federico-G commented Sep 20, 2018

  • C3 version: v0.6.7
  • D3 version: v5.7.0
  • Browser: Chrome 69 & Firefox 62
  • OS: Ubuntu 16.04 (updated)

@mageshveeraa I confirm the bug, and I think that multi level zoom should be the behavior

@kt3k @mistic I also found that zoom.enable is not working with this type of zoom

You can try it in https://c3js.org/samples/interaction_zoom_by_drag.html
In the console, paste chart.zoom.enable( false );. It doesn't do anything

Thanks

@mageshveeraa
Copy link

@mageshveeraa mageshveeraa commented Sep 21, 2018

@EliasC10 No, I didn't find the solution yet. Now I am using zoom by scroll only.

@Federico-G Yes, I mean multi-level zoom behavior is not working.

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

Successfully merging this pull request may close these issues.

None yet

6 participants