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

animated svg on homepage #331

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

animated svg on homepage #331

wants to merge 59 commits into from

Conversation

johangiraud
Copy link
Contributor

Ok, I gave a first try for svg simple animation. Now it's working but it still need to be improved a lot

@johangiraud johangiraud changed the title add animated svg on category display homepage animated svg on homepage Mar 9, 2018
@vinyll
Copy link
Member

vinyll commented Mar 12, 2018

Should this be reviewed?

@johangiraud
Copy link
Contributor Author

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.


/* animation picto sections */
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => {
window.addEventListener('scroll', function() {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes !

/* animation picto sections */
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => {
window.addEventListener('scroll', function() {
if (svg.getBoundingClientRect().top < 600) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe document why 600?

Copy link
Contributor Author

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

Copy link
Member

@vinyll vinyll left a 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.


/* animation picto sections */
Array.from(document.querySelectorAll('.svg-animation')).forEach(svg => {
window.addEventListener('scroll', function() {
Copy link
Member

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: …,() => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@vinyll vinyll force-pushed the master branch 2 times, most recently from 3cc4e3c to cf999c7 Compare March 18, 2019 16:05
@vinyll vinyll force-pushed the master branch 7 times, most recently from 056d1f6 to 8dfe1d6 Compare November 28, 2019 12:03
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.

3 participants