-
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
Gsap animation #17
Gsap animation #17
Conversation
How does it close #10? We won't do it for now, but it can be in the "after-dekrook" part right? |
src/redux/actions.js
Outdated
@@ -19,8 +18,3 @@ export const listenToQueries = () => ( | |||
}); | |||
} | |||
); | |||
|
|||
export const deleteVisible = (index) => ({ |
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.
Didn't read everything, but how do you delete the visible ones after x amount of time? do they stay in the DOM now? :(
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.
They do but i max them at 40, after that they get trimmed. I can do it more elegantly but the perf doesnt really suffer I think. I will probably let them expire after their animation again after i clean up Pod.js etc.
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.
ah trimming at 40 makes perfect sense and is an easier implementation. Will cause an edge case when there are more than 40 supposed to be visible though 👍
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.
Im probably going to write another action that will fire after the gsap animation completes
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.
it should probably dispatch after x amount of seconds after the gsap animation
src/redux/reducers.js
Outdated
@@ -35,11 +31,6 @@ export const queryReducer = (state = INITIAL_STATE, action) => { | |||
all = all.slice(0, 20); | |||
} | |||
|
|||
// VISIBLE_INDEX++; | |||
// setTimeout((index) => { | |||
// store.dispatch(deleteVisible(index)); |
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.
I don't get why it's not needed anymore
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.
there is no more visible field in the store, instead i use queries.all to render the pods.
Unlimited pods on screen at once.
src/components/Scene.js
Outdated
@@ -61,10 +61,10 @@ class Scene extends Component { | |||
renderPods() { | |||
const {queries, removePod} = this.props; | |||
return queries.all | |||
.map((query, i) => ( |
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.
map((query,index)) => (
<Pod
key={index}
/>
this will work without needing to keep track of a specific index
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.
An array/object always already has an internal index
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.
It causes a bug because the index of the pods changes as they are deleted/inserted.
Pods will randomly dissappear while they are animating, that's why I changed it.
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.
dommerik
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.
I believe you 😉
@@ -32,12 +32,16 @@ class App extends Component { | |||
this.props.listenToQueries(); | |||
} | |||
|
|||
removePod = (query) => { |
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.
I'd use
removePod(query) {
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.
this notation is used to bind this implicitly (see https://medium.com/@housecor/react-binding-patterns-5-approaches-for-handling-this-92c651b5af56#.tpfyexc3z, option 5)
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.
Good point, didn't think of "this"
src/components/Pod.js
Outdated
[curve, z, scaleEnd] = [[...hub.paths.back_right.bezier].reverse(), 2, 0.33]; | ||
break; | ||
case DIRECTIONS.southwest: | ||
[curve, z, scaleEnd, west] = [[...hub.paths.front_left.bezier].reverse(), 2, 2, true]; |
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.
Would an object be clearer?
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.
I like this notation more because I don't have to write () around it every time. I don't think it makes a difference anyway.
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.
I prefer named arguments, but let's not make an argument about that now :^-)
New animation for the pods that follows the path of the roads.
fixes #9 and #16
I removed the buzz animation for now.
closes #12