Skip to content

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.

4 participants