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

Pan Zoom #2

Merged
merged 4 commits into from Jan 2, 2019
Merged

Conversation

troygibb
Copy link

@troygibb troygibb commented Dec 19, 2018

Found your plugin super useful! I needed the ability to pan a zoomed portion of the branch, so I essentially reused your doZoom method and exposed the panZoom functionality on the chart instance. There should be no change to the existing doZoom method other than adding those properties to the crosshair object.

I bumped the minor version manually--I don't know your deployment scheme, so I can take this commit out if needed.

(Side note: sorry for the whitespace changes in /samples/index.html, my linter had its way with that file. Figured I'd leave it given it was taking queues from your eslintrc).

@AbelHeinsbroek
Copy link
Owner

Thank you for your PR! I'm glad you like this plugin and find it useful.

I would prefer refactoring the panZoomLeft() and panZoomRight() functions to panZoom(-offset) and panZoom(+offset), and to remove the pan.incrementer option altogether. This will allow for example for a set of buttons below the chart with different pan increments/decrements (-1M, -1H, -1D, +1D, +1H, +1M)

In addition, we need to add support for adding moment.js durations as well to support charts with a time axis.

Agreed?

@troygibb
Copy link
Author

@AbelHeinsbroek thanks for the quick response! Agreed on refactoring the panzoom api--just pushed up those changes.

I also noticed that if you don't add data to a chart js instance immediately, chartjs-plugin-crosshair will err out occasionally as afterEvent callbacks are fired. I put a fix here.

As for the moment duration changes, I'm having a bit of trouble getting zoom functionality working. I pushed up an example in samples/index.html here without my newest changes. I noticed that the filtering bit is comparing unlike types, e.g:

...
if (oldData.x >= start && !started && index > 0) {
  newData.push(sourceDataset[index - 1]);
  started = true;
}
...

In this example, start has been resolved to a moment object, but oldData.x is resolving to just a date string. Is my example implemented incorrectly? Although it seems that chartjs has no problem with the data I'm adding.

@AbelHeinsbroek
Copy link
Owner

In this example, start has been resolved to a moment object, but oldData.x is resolving to just a date string. Is my example implemented incorrectly? Although it seems that chartjs has no problem with the data I'm adding.

That's because you enter strings as input in your example:

  function newDateString(days) {
      return moment()
        .add(days, "d")
        .format();
  }

If you pass in the moment object directly it works fine:

  function newDateString(days) {
      return moment()
        .add(days, "d")
  }

@troygibb
Copy link
Author

Hm ok--it looks like moment.durations is supported out of the box here! I had to take out a type check on increments being of type number, but I think we should be good to go here!

Can you confirm when you get a chance?

@troygibb
Copy link
Author

@AbelHeinsbroek Any chance you've had a moment to look at this?

@AbelHeinsbroek
Copy link
Owner

Looks good to me!

@AbelHeinsbroek AbelHeinsbroek merged commit c1700af into AbelHeinsbroek:master Jan 2, 2019
@troygibb
Copy link
Author

troygibb commented Jan 2, 2019

Awesome! thanks again :)

@troygibb
Copy link
Author

troygibb commented Jan 2, 2019

@AbelHeinsbroek fyi I'm not seeing the package updated with the latest change--any chance you could republish?

@AbelHeinsbroek
Copy link
Owner

Thanks, forgot to run gulp before publishing.

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.

None yet

2 participants