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

README refresh, disconnect observer, observer options #146

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

Conversation

leastbad
Copy link
Contributor

@leastbad leastbad commented Jul 9, 2022

Type of PR (feature, enhancement, bug fix, etc.)

Enhancement

Description

This PR disconnects each IntersectionObserver after it triggers the content generation request. Otherwise, the callback will keep firing and generate load on the server. It's also not efficient to have lots of useless observers in scope.

The futurize helper now accepts an optional observer_options parameter. It creates an options hash that allows you to set a rootMargin greater than the size of the window.

The root module now exports an initialize(consumer) method, which eliminates the need to call two methods with longer names.

Finally, I updated the README to reflect the new parameter, and wrote a bit more about the syntax. I did remove the many occurances of extends: :div because it's the default you'll use 90% of the time.

Why should this be added

These changes serve my desire to use Futurism to create the endless scroll demo that shames other endless scroll techniques. 😉

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing

@leastbad leastbad added documentation Improvements or additions to documentation enhancement New feature or request javascript Pull requests that update Javascript code labels Jul 9, 2022
@leastbad leastbad requested a review from julianrubisch July 9, 2022 07:53
@julianrubisch
Copy link
Contributor

Nice! It will take some time for me to put this through its paces but definitely an improvement.

lib/tasks/futurism_tasks.rake Outdated Show resolved Hide resolved
lib/futurism/helpers.rb Outdated Show resolved Hide resolved
javascript/elements/futurism_utils.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@leastbad
Copy link
Contributor Author

I'm having trouble figuring out why the tests are failing / making the tests pass, and I don't think it's because of my PR that they are failing?

cable_ready_channel.expect(:outer_html, nil, [Hash])

It seems like you have a few repeated lines here, but the real truth is that I have very little direct experience working with mocked objects. If you have any suggestions, I'm happy to roll up my sleeves but I figured a quick sanity check-in first was the move.

@julianrubisch
Copy link
Contributor

Just wanted to drop that I haven't forgotten about this. I will take a look at the test failures and come up with more feedback soon.

@leastbad
Copy link
Contributor Author

What is time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request javascript Pull requests that update Javascript code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants