-
Notifications
You must be signed in to change notification settings - Fork 1
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
animated svg on homepage #331
base: master
Are you sure you want to change the base?
Conversation
Should this be reviewed? |
I'll be glad to have you mind on the question. But since in not in the sprint, I don't ask for review. It was more because we talked about it @vinyll, and you told me it was not a problem to add PRs aside. |
cafebabel/static/js/script.js
Outdated
|
||
/* animation picto sections */ | ||
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => { | ||
window.addEventListener('scroll', function() { |
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.
You can use window.addEventListener('scroll', () => {
which is more consistent.
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.
yes !
cafebabel/static/js/script.js
Outdated
/* animation picto sections */ | ||
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => { | ||
window.addEventListener('scroll', function() { | ||
if (svg.getBoundingClientRect().top < 600) { |
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.
Maybe document why 600
?
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.
Yea I didn't know how to get the moment where the picto start being watched. Maybe viewportHeight
will do d457cd0
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'm ignorant about this topic.
The code is good for me tho.
I would unlock this and let @davidbgk recomment if required.
cafebabel/static/js/script.js
Outdated
|
||
/* animation picto sections */ | ||
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => { | ||
window.addEventListener('scroll', function() { |
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.
es6 arrow function for consistency: …,() => {
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.
👍
3cc4e3c
to
cf999c7
Compare
056d1f6
to
8dfe1d6
Compare
Ok, I gave a first try for
svg
simple animation. Now it's working but it still need to be improved a lot