-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
Add #5 because I think it is premature to do the additional filtering especially since we will have access to that information |
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 |
✔️ 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/ |
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 |
There was a problem hiding this 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?
Yes, that has now been recorded in issue #8 |
There was a problem hiding this 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
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: