Is able to past list-cols to timevis (#11)

[strazto]

Only two months later I took the 10 minutes it takes to checkout your last commit, choose a clean working tree and squash my commits.... Lmao sorry @daattali

A bunch of meaningless commits that will be squashed

• Added glue to suggests

• Changed encoding function to produce json objects

• Added helper for more informative test output

• Added calls to info output helper

• Only writes list-cols as json

Passes regression tests again

• Added unit test to test nested col behaviour

(Failing)

• new test now passing

• Now passes list-cols (such as nestedGroups) through

• Update version number

• Added .DS_Store to gitignore

• Added newest release of vis.js

• Update timevis.yaml to point at vis-4.21

• Added .DS_Store to .gitignore

• Updated unit test

• commented info comp test helper

• Added unit test to check length-1 listing

• Added list check to dataframetod3

• added null handle to dftod3

• Commented null handle out in dataframetoD3

Mainly because I should probably write more regression tests for it before blindly putting it in
I should probably implement a check on the whole column, rather than checking in an apply to check if any are vectors in the col

• Added timeline plus

• Updated dependencies to use timeline-plus

• renamed timeline-plus back to vis, setup src properly

• Update timevis.yaml

• Changed references from vis to timeline

• Update timevis.yaml

• Revert "Changed references from vis to timeline"

This reverts commit d7c169e.

• changed all refs to timeline to vistime because of collision

• replaced references to vis with timeline

• updated checks in dataframetod3 coercion

added a check for logical

• Replaced timeline.js with my own fixed version

• Extra test case to cover when empty list passed

• dftod3 Passes empty lists and null as NA

• Revert "Merge pull request timevis doesn't work when the input dataframe is a merged/row-binded dataframe #7 from mstr3336/upgrade-timeline-plusplus"

This reverts commit 38bd4e4, reversing
changes made to 9050114.

[daattali]

Do you remember why this is useful?

[strazto]

I don't exactly remember why it was useful, but I do remember that there's some kind of property that timevis.js accepts as a vector.

It's groups, or nesting levels, or something?

And fixing this allowed it to work.

[strazto]

Here's why it's useful: #68

[daattali]

Ah great find, thank you. Unfortunately I'm still holding back on upgrading visjs because they have a few bugs that I reported several times but haven't been fixed yet, and I don't want to introduce regression bugs into this package.

Let me know if you find out the reason for your other PR - to see if it helps support a feature that should work with the current version!

[strazto]

Hi @daattali , I've put in PRs to address those regression bugs in vis-timeline-
(One is certainly a bug, & the other may fall under the classic "Not a bug, a feature" category)

I'm going to clean this PR up, particularly the superfluous dependency additions.

I'm closing #74 , as this is just a squash of #74, which saves us from needing eyebleach after a million trial-and-error commits & reverts

[daattali]

Since the new version of vis-timeline contains many new config options, and you're just started moving the gears towards having that happen, what do you think about holding off on this until the new version is used so that we can test this list-col with the new configs?

[strazto]

I can tell you that I've been using my fork, with a bundled updated vis-timeline for years now, so I'm fairly satisfied with this aspect of functionality

[daattali]

Ok. I just submitted a new version last week, so this will be done for the next version which will be a major release

…

On Thu., Oct. 1, 2020, 11:57 Matthew Strasiotto, ***@***.***> wrote: I can tell you that I've been using my fork, with a bundled updated vis-timeline for years now, so I'm fairly satisfied with this aspect of functionality — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHIQFHML4FYHLMO7XWC25TSISRG7ANCNFSM4HCQTHPA> .

[strazto]

Ok. I just submitted a new version last week, so this will be done for the next version which will be a major release

Great! I'd just like to clarify the timeframe & deployment of that -

Once the vis-timeline maintainers review my work / merge it, you'll then be happy to bundle the head of vis-timeline, & update it for this repo

Once you bundle that, you're happy to merge this PR.

When that's done, you'll bump the major version & submit to CRAN

( P.S I'm not sure if this warrants an entire major release, I don't think that it should break any existing APIs, although I suppose the extensive changes in vis-timeline are likely to break at least something)

[daattali]

Even if they don't merge your fixes I'll still merge this one. But I spent a long time this month on this package and a few others, so I ended to focus on other packages before I come back to this one. I will make it a major release because updating the library from 4.16 to the current version is a huge change, and it is bound to have some breaking changes even if I dont know what they are. The two that I mentioned are just two simple ones that i found easily, but there are probably more if you do rigorous testing.

[daattali]

@matthewstrasiotto I (finally) updated timevis to use the latest visjs timeline (v7.4.9). If you're still around, would you like to come back to this PR and make sure it works with the new version?

[strazto]

I'll try to get to that!

[daattali]

@matthewstrasiotto do you want to take another stab at this?

[JoshRosenstein]

@daattali @matthewstrasiotto I did a quick visual test and confirmed that I was able to reproduced nestedGroupsExample with the proposed changes to dataframeToD3 with the current master branch.

[daattali]

@JoshRosenstein have you been able to come up with a short simple example of using this feature? Before adding and announcing this this feature I'd like to be able to also add a nice example of using nestedGroups. Ideally the entire data passed to timevis() could be contained in one call, eg.

timevis(data = data.frame(...), start = c(...), content = c(...), group = c(...), groups = data.frame(...))

The unit tests that were added show that it's a little complex to create a dataframe with one of its values being a list; I'm not an expert on list-columns so I'm wondering if that's just a reality in R that it's not possible to create such a dataframe in one call?

[strazto]

The main thing I took from reviewing my unit tests was that I was 23 years old when I first submitted this PR & now I'm nearly 27 lol
https://github.com/daattali/timevis/pull/77/files#diff-e9e4452695104af0ad5ad91ae829925d3d34628e9cd172e5bfd39a726fb10a72R52

Pretty sure you can create a list-col inline, something like:

  df <- data.frame(name = c("Dean", "Matt"),
                   age = c(31, 26),
                   hobbies = list(c("Merging matt's Prs", "Other stuff"), c("Volleyball", "Rocket League")))

Regardless of how they're made in R, list cols exist, have use-cases, and have a valid encoding to d3.

I don't think I have access to the code-bases I was working on when I first created this PR unfortunately, or I'd pull something more concrete from them

[daattali]

Yeah this is definitely the longest PR it ever took me to merge. 2.5 years were wasted because the visjs guys ignored the regression bugs the new version caused and never fixed it. After I finally deicded to take a few days to understand all the breaking changes and fix them on {timevis} end, I came back to this PR but wasn't sure what's the purpose of it until 2 weeks ago. Thanks to @JoshRosenstein for mentioning that this is needed for nested group support :)

I do want to merge this, I just said that before adding it I'd like to also include a simple easy to understand example of how to use it. I get a few emails every week asking me to show how to use some feature of timevis so I think it's very important to show example usage of non trivial features. Unfortunately the code you provided doesn't work:

> data.frame(
+     name = c("Dean", "Matt"),
+     age = c(31, 26),
+     hobbies = list(c("Merging matt's Prs", "Other stuff"), c("Volleyball", "Rocket League"))
+ )
  name age hobbies.c..Merging.matt.s.Prs....Other.stuff.. hobbies.c..Volleyball....Rocket.League..
1 Dean  31                             Merging matt's Prs                               Volleyball
2 Matt  26                                    Other stuff                            Rocket League

I do consider it mandatory to include example usage with a feature, it's not complete without it!

[strazto]

To be clear, I didn't intend my previous comment to have a rude tone, and I certainly remember the problems with vis.js (Those libraries were a mess). It just feels strange to look back at what's probably one of the first open source contributions I ever made

I think Josh will have a better shot at rounding out the PR, as I no longer even program in R, and the context + mechanics of this are very far from me

@JoshRosenstein, if you want to base off of my branch, you can either ping me and I'll pull your changes onto this PR, or you can submit a separate PR, whatever's easier all round

[strazto]

As for the list col thing, I think I needed to wrap it in I() because base R likes to do weird things to your inputs

  df <- data.frame(
    name = c("Dean", "Matt"),
    age = c(31, 26),
    hobbies = I(list(
     c(
       "Merging matt's Prs",
       "Other stuff"), 
     c(
       "Volleyball",
       "Rocket League")
    ))
  )

print(df$hobbies)
print(df)

Some online repl gives this:

[[1]]
[1] "Merging matt's Prs" "Other stuff"       

[[2]]
[1] "Volleyball"    "Rocket League"

  name age      hobbies
1 Dean  31 Merging ....
2 Matt  26 Volleyba....

[daattali]

Ah yes, I() is the solution here! OK I'll see if I can get an example of nested groups to work and then finally close the loop on your first PR

[daattali]

Merged, thank you!

[daattali]

Example using nested groups:

timevisNestedData <- data.frame(
  id = 1:14,
  content = c(
              "Open", "Open", "Half price entry",
              "Open", "Adults only", "Open", "Staff meeting",
              "Open", "Men only", "Women only", "Open",
              "Hot water", "Very hot water",
              "Siesta"),
  start = c(
            "2016-05-01 06:00:00", "2016-05-01 14:00:00", "2016-05-01 08:00:00",
            "2016-05-01 08:00:00", "2016-05-01 14:00:00",
            "2016-05-01 16:00:00", "2016-05-01 19:30:00",
            "2016-05-01 08:00:00", "2016-05-01 14:00:00", "2016-05-01 16:00:00", "2016-05-01 18:00:00",
            "2016-05-01 09:00:00", "2016-05-01 14:00:00",
            "2016-05-01 12:00:00"),
  end   = c(
            "2016-05-01 12:00:00", "2016-05-01 22:00:00", "2016-05-01 10:00:00",
            "2016-05-01 12:00:00", "2016-05-01 16:00:00",
            "2016-05-01 20:00:00", NA,
            "2016-05-01 12:00:00", "2016-05-01 16:00:00", "2016-05-01 18:00:00", "2016-05-01 20:00:00",
            "2016-05-01 12:00:00", "2016-05-01 17:00:00",
            "2016-05-01 14:00:00"),
  group = c(rep("gym", 3), rep("pool", 4), rep("sauna", 4), rep("tub", 2), NA),
  type = c(rep("range", 6), "point", rep("range", 6), "background")
)
timevisNestedDataGroups <- data.frame(
  id = c("gym", "pool", "sauna", "tub"),
  content = c("Gym", "Pool", "Sauna", "Hot Tub"),
  nestedGroups = I(list(NA, list("sauna", "tub"), NA, NA))
)
timevis(timevisNestedData, timevisNestedDataGroups)

[JoshRosenstein]

@daattali @strazto - Apologize for missing these notifications. Glad you got this baby merged! Thank you. 2.5 years for this PR... I've been itching for this to work 5+ years ago 😆. Came across the issue I had created with my other GH account #32 back in 2017 🤣