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.

Sign up for GitHub

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

Jump to bottom

feat(www): add 'lead' animation to the Ecosystem horizontal scrollers #9606

Merged
merged 8 commits into from
Nov 7, 2018
Merged

feat(www): add 'lead' animation to the Ecosystem horizontal scrollers #9606

merged 8 commits into from
Nov 7, 2018

Conversation

greglobinski
Copy link
Contributor

@greglobinski greglobinski commented Oct 31, 2018 •
edited
Loading

  • install intersection-observer polyfill
  • refactor ecosystem-board.js to class component to manage IntersectionObserver
  • setup IntersectionObserver to observe horizontal scrollers
  • add style class with initial 'lead' animation

Video preview at Slack

@greglobinski greglobinski requested a review from a team as a code owner October 31, 2018 16:52
DSchau
DSchau reviewed Oct 31, 2018
View reviewed changes
Copy link
Contributor

@DSchau DSchau left a comment

Choose a reason for hiding this comment

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

Looking good! Left a few comments.

www/src/components/ecosystem/ecosystem-board.js Outdated Show resolved Hide resolved
www/src/components/ecosystem/ecosystem-board.js Outdated Show resolved Hide resolved
www/src/components/ecosystem/ecosystem-board.js Outdated Show resolved Hide resolved
www/src/components/ecosystem/ecosystem-board.js Show resolved Hide resolved
www/src/components/ecosystem/ecosystem-board.js Outdated Show resolved Hide resolved
@greglobinski
Copy link
Contributor Author

@DSchau @amberleyromo The longer I think of it I'm more and more sure that I should strip out the intersection-observer and do it the old way, by listening to scroll event and comparing the offsets of page and elements. What do you think? Did I make mistake?

@DSchau
Copy link
Contributor

DSchau commented Nov 1, 2018

@greglobinski it's totally doable, but personally, I prefer to treat a feature like this, which isn't strictly speaking necessary, as an enhancement. If it doesn't work in Safari, that's not great but I don't think it's necessarily a huge issue nor a significantly diminished experience.

Additionally, I think manually wiring up intersection observer like behavior (with scroll listeners) would work, but I just think it would introduce some code that I personally wouldn't really want to maintain. I think the value of keeping the code pretty clean and concise with IntersectionObserver is worth the trade off of it not working in every browser.

jlengstorf
jlengstorf reviewed Nov 3, 2018
View reviewed changes
Copy link
Contributor

@jlengstorf jlengstorf left a comment

Choose a reason for hiding this comment

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

Looking great, @greglobinski!

@greglobinski
Copy link
Contributor Author

greglobinski commented Nov 7, 2018 •
edited
Loading

@amberleyromo

  • remove all "fake scroll" staff (JS and CSS code manging animation styles)
  • implement real scrolling with ease function
  • add some delay (250ms) before launching animation for better performance

@greglobinski greglobinski mentioned this pull request Nov 7, 2018
Merged
@amberleyromo amberleyromo merged commit 92593af into gatsbyjs:master Nov 7, 2018
@greglobinski greglobinski deleted the gl/ecosystem/scroll branch November 15, 2018 22:15
gpetrioli pushed a commit to gpetrioli/gatsby that referenced this pull request Jan 22, 2019
@greglobinski @gpetrioli
feat(www): add 'lead' animation to the Ecosystem horizontal scrollers (…
6c55563 
…gatsbyjs#9606)

* feat(www): add lead scroll to horizontal scrollers

feat(www): setup IntersectionObserver

feat(www): add initial lead animation to horizontal scrollers on Ecosystem page

fix(www): remove intersection-observer polyfill

fix(www): change fake scrolling to 'only left' ver

fix(www): make 'fake scroll' not fake

* fix(www): add code formating

* fix(www): add code formating and fix eslint errors

* fix(www): add small delay for lead animation

* fix(www): decrease dalay to 250ms

* refactor(www): refactor easy function

* fix(www): revert to overriden proper Ecosystem board texts

* fix(www): remove unused CSS style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlengstorf jlengstorf jlengstorf left review comments

@pieh pieh pieh left review comments

@DSchau DSchau DSchau left review comments

@amberleyromo amberleyromo amberleyromo approved these changes

@fk fk Awaiting requested review from fk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@greglobinski @DSchau @jlengstorf @pieh @amberleyromo