search

daattali / timevis

📅 Create interactive timeline visualizations in R
http://daattali.com/shiny/timevis-demo/
Other
655 stars 157 forks source link

minor documentation change #42

Closed greg-minshall closed 5 years ago

greg-minshall commented 6 years ago

just the two files. hopefully okay. (a few extra changed-by-roxygen2-i-guess lines in man/timevis.Rd.)

daattali commented 6 years ago

there seem to be 2 separate issues in this PR, something about leaking information?

daattali commented 6 years ago

Have you tested this upgrade to 4.21? Every time I spend half an hour trying to upgrade timevis, I find various bugs. It's probably fixable but some of the R/JS code in the package would have to change and I haven't figured it out yet

greg-minshall commented 6 years ago

Dean, are my most recent commits to my repository coming through on the old pull request? apologies if so!

i am playing with 4.21, having wanted onIntialDrawComplete (but am not yet sure how to pass the "function" argument). but, i didn't intend for my playing to escape.

is there a test suite that would show what you run into? for my simple use, 4.21 appears fine.

daattali commented 6 years ago

Try running the example app (the one under inst/) and see that everything works properly

On Feb 21, 2018 01:58, "greg-minshall" notifications@github.com wrote:

Dean, are my most recent commits to my repository coming through on the old pull request? apologies if so!

i am playing with 4.21, having wanted onIntialDrawComplete (but am not yet sure how to pass the "function" argument). but, i didn't intend for my playing to escape.

is there a test suite that would show what you run into? for my simple use, 4.21 appears fine.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/daattali/timevis/pull/42#issuecomment-367231452, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFJB5WIx2u5nhoNbFmPMK7kpW6LMKks5tW76QgaJpZM4Rd8lL .

greg-minshall commented 6 years ago

so, problem appears to be that in vis.js/Timeline 4.16.1, a date/time could look like

2016-10-20 16:35

but in 4.21.1 (whatever), it needs to have a "T" instead of a space, i.e.,

2016-10-20T16:35

if you want a pull request, i'd be happy to (a minor couple of documentation changes to go along with it). you might want to complain to vis.js/Timeline, though?

daattali commented 6 years ago

Wow is that really the source of the difference? Thanks for debugging that. I'll test that out and definitely submit an issue to visjs!

I'd be happy to get a PR! And even happier if it also fixes the existing code examples (in the documentation and the sample app in inst/) :)

On Feb 28, 2018 08:24, "greg-minshall" notifications@github.com wrote:

so, problem appears to be that in vis.js/Timeline 4.16.1, a date/time could look like

2016-10-20 16:35

but in 4.21.1 (whatever), it needs to have a "T" instead of a space, i.e.,

2016-10-20T16:35

if you want a pull request, i'd be happy to (a minor couple of documentation changes to go along with it). you might want to complain to vis.js/Timeline, though?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/daattali/timevis/pull/42#issuecomment-369237962, or mute the thread https://github.com/notifications/unsubscribe-auth/AA6IFAOl-sj1ApGmNp-6H7DBAXkageWGks5tZVOegaJpZM4Rd8lL .

greg-minshall commented 6 years ago

okay, there should be a pull request. please critique, if criticism there be (especially the git bits)!

daattali commented 6 years ago

The title of this PR is "minor documentation change" - this is actually a PR for upgrading to visjs 4.21, correct?

daattali commented 6 years ago

@greg-minshall are you sure the T issue is all you need to fix visjs 4.21? Did you test this out with the visjs old and new javascript versions? I just tried running a very simple HTML page with a visjs timeline example, using visjs 4.21, and using the old date string (without T) and it worked just fine.

daattali commented 6 years ago

It looks like this time thing is only an issue in RStudio , that's why I didn't see it when I tested an HTML page.

I made an issue for upgrading visjs https://github.com/daattali/timevis/issues/50 and listed there all the outstanding regression bugs. There are two issues that are not solvable on my end, they are bugs I reported to visjs

greg-minshall commented 6 years ago

right. my "testing" is flawed in that i don't use shiny.

daattali commented 5 years ago

@greg-minshall I'm doing some cleanup in this repo and closing this PR. After thinking about this some more and discussing with several people, it does not seem that "warning" users that the dataframe they pass into the function is available in javascript is necessary

greg-minshall commented 5 years ago

obviously, i disagree. but, good luck with the upgrades, etc.!

(oddly, after some time, i've just started up a personal project that will likely use timevis!)

daattali commented 5 years ago

I completely understand your disagreement. I'm adding some other improvements to the package, I hope you'll still find it useful