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

Time splitting #4

Merged
merged 21 commits into from
Jan 13, 2022
Merged

Time splitting #4

merged 21 commits into from
Jan 13, 2022

Conversation

gotdairyya
Copy link
Collaborator

@gotdairyya gotdairyya commented Jan 6, 2022

Does this PR close any open issues?

Closes #2
Closes #1

Give a longer description of what this PR addresses and why it's needed

This PR allows the user to split the network according to a set number of time slices.

Provide pictures/videos of the behavior before and after these changes (optional)

Are there any additional TODOs before this PR is ready to go?

TODOs:

  • Filter network based on the edge attribute selections

@gotdairyya
Copy link
Collaborator Author

Add #5 because I think it is premature to do the additional filtering especially since we will have access to that information

@gotdairyya gotdairyya marked this pull request as ready for review January 8, 2022 04:56
@gotdairyya gotdairyya requested a review from JackWilb January 8, 2022 04:56
@JackWilb
Copy link
Member

Looks like a good start. I'm interested to see if we can simplify the code a little, by reusing code for the controls and legend charts in a smarter way. If you have time to do some digging, that'd be awesome. If not, we can keep a note of it for the future, but we will be accruing some technical debt.

I'm requesting changes for the small changes and to see how you feel about removing some of the duplicated code. Let me know what you think

src/types.ts Outdated Show resolved Hide resolved
src/App.vue Outdated Show resolved Hide resolved
src/components/DragTarget.vue Outdated Show resolved Hide resolved
src/components/TimeLine.vue Outdated Show resolved Hide resolved
src/components/TimeLine.vue Outdated Show resolved Hide resolved
src/components/TimeSlicing.vue Outdated Show resolved Hide resolved
src/components/TimeSlicing.vue Outdated Show resolved Hide resolved
src/components/TimeSlicing.vue Show resolved Hide resolved
src/components/EdgeBuilder.vue Show resolved Hide resolved
src/components/EdgeBuilderChart.vue Show resolved Hide resolved
@netlify
Copy link

netlify bot commented Jan 12, 2022

✔️ Deploy Preview for multidynamic ready!

🔨 Explore the source changes: 0a906c9

🔍 Inspect the deploy log: https://app.netlify.com/sites/multidynamic/deploys/61e076afa79c4f000851a370

😎 Browse the preview: https://deploy-preview-4--multidynamic.netlify.app/

@gotdairyya
Copy link
Collaborator Author

I created a new issue #7 so that we can merge to main and I can get feedback from collaborators while working on simplifying the code

@gotdairyya gotdairyya requested a review from JackWilb January 12, 2022 20:55
Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 comments to resolve still. I also want to make a note that we're duplicating the controls logic, too, for some of these components. Would you open an issue for that?

@gotdairyya
Copy link
Collaborator Author

2 comments to resolve still. I also want to make a note that we're duplicating the controls logic, too, for some of these components. Would you open an issue for that?

Yes, that has now been recorded in issue #8

@gotdairyya gotdairyya requested a review from JackWilb January 13, 2022 18:04
Copy link
Member

@JackWilb JackWilb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks Derya

@gotdairyya gotdairyya merged commit cd0b2d7 into main Jan 13, 2022
@gotdairyya gotdairyya deleted the time-splitting branch January 18, 2022 20:36
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.

Fix selectedRange Fix slice rules to check for type and show hints
2 participants