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

fix(timeline.setgroups): nested groups fold correcly when input is array #718

Merged
merged 4 commits into from
Oct 24, 2020

Conversation

strazto
Copy link
Contributor

@strazto strazto commented Oct 13, 2020

Squash preferered, HEAD is a revert of a commit that added a demo

Fixes #113

checkout 1a8fd7e to see demonstration, head doesn't include this as the example isn't intended for production, and I want to expedite the review.

@melloware
Copy link
Contributor

looks like you have a conflict?

@strazto
Copy link
Contributor Author

strazto commented Oct 23, 2020

Yeah, i guess in the last fortnight there's been some changes.
I'd prefer the reviewer address such conflicts, if possible

@melloware
Copy link
Contributor

I don't know about this project but typically that is not how it works. You would rebase this branch and get your PR working. Or close this PR and open another one on the latest version of the code with your fixes again. Just my two cents...

@strazto
Copy link
Contributor Author

strazto commented Oct 23, 2020

Yeah, you're right
I'll get around to it when I'm at a computer, I just have a few PRs open against master and I dont want to update them if they're going to collect dust

@strazto
Copy link
Contributor Author

strazto commented Oct 24, 2020

Hm, the conflict looks non-trivial - I don't really understand its purpose, but it's possibly intended to address the same behaviour I am. I'll check the commit log

@strazto
Copy link
Contributor Author

strazto commented Oct 24, 2020

@Thomaash this conflicted with your commit 36e1482 #729

I'd just like to confirm with you that my merge looks okay to you?

Also @yotamberk or any maintainer, when you get the chance, can you please review this?

@strazto strazto force-pushed the fix_bare_array_group_nests branch from 3649858 to 840ca81 Compare October 24, 2020 03:44
@strazto strazto force-pushed the fix_bare_array_group_nests branch from 840ca81 to afe98c5 Compare October 24, 2020 03:47
@yotamberk yotamberk merged commit 97d3f16 into visjs:master Oct 24, 2020
@vis-bot
Copy link
Collaborator

vis-bot commented Oct 24, 2020

🎉 This PR is included in version 7.4.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Thomaash
Copy link
Member

Hi @matthewstrasiotto,

I don't think you changed my intended behavior in any meaningful way. However, even though I didn't respond on time, please don't use instanceof Array. The problem is that if the array comes from any other context (iframes, web workers etc.) it won't have the same Array in it's prototype chain and won't pass this check even though it is an array and works as one just fine. The function Array.isArray(…) would work much better.

@strazto
Copy link
Contributor Author

strazto commented Oct 24, 2020

Ah, crap, thanks for the comment @Thomaash - I'm not a JS dev, so I usually just replicate the patterns I see in the codebases I'm working on.
Probably deserves its own PR to address that behaviour

@Thomaash
Copy link
Member

Ah, crap, thanks for the comment @Thomaash - I'm not a JS dev, so I usually just replicate the patterns I see in the codebases I'm working on.
Probably deserves its own PR to address that behaviour

Yeah, it was already in the codebase. I solved all of them in #750.

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

Successfully merging this pull request may close these issues.

Update GitHub templates
5 participants