search

jlew715 / tc359_rails_final

0 stars 0 forks source link

Infinite scroll skill complete #4

Open jlew715 opened 9 years ago

jlew715 commented 9 years ago

@chrisvfritz

As we discussed last week, I've added an "infinite scroll" feature to the cats page, where scrolling to the bottom loads more cat pictures.

chrisvfritz commented 9 years ago

Very good - except it does look like there's still some cleanup to do. I see you've added a moreCats action to your static controller and a jQuery plugin for infinite scroll. You're not using either of them for your final solution, so it's best to remove them, to keep your project from cluttering up. Do that cleanup and I'll be happy to approve.

As an aside - albeit an important one - nothing in Ruby (and especially Rails, as Rails has some magic that depends on the conventions) should ever be named in camelCase. If it's a variable or method name, it should be all lowercase with underscores (e.g. more_cats); if it's a module or class name, it should be in TitleCase (e.g. StaticController); if it's a constant, it should be all uppercase with underscores (e.g. THIS_IS_A_CONSTANT).

chrisvfritz commented 9 years ago

And one other piece of feedback. This isn't something you have to update, but I did a quick refactor of your code in CoffeeScript, to give you a taste of the power of CoffeeScript and also help you learn how to implement a slightly more powerful infinite scroll.

Don't hesitate to let me know if you have any questions/comments. :smiley: 

# CoffeeScript classes rock for modularizing our code!

class InfiniteCatGenerator

  # ---------
  # CONSTANTS
  # ---------

  # These are constants are unlikely to ever change.

  $WIN = $(window)
  $DOC = $(document)

  # ----------------------
  # CONGIGURABLE CONSTANTS
  # ----------------------

  # These are constants are likely to change as we decide
  # to tweak the behavior of the infinite scrolling.

  IMG_CLASS = 'cat-picture'
  NUMBER_OF_CATS_TO_APPEND = 5
  PIXELS_FROM_BOTTOM_TO_PULL_NEW_CATS = 500
  CATS_ENDPOINT = 'http://thecatapi.com/api/images/get?format=src'

  # ----------------
  # GLOBAL VARIABLES
  # ----------------

  # These are variables that should be accessible in all our functions

  $container = undefined
  loading_new_cats = false

  # The constructor function is automatically run whenever a new
  # InfiniteCatGenerator is created

  constructor: (cats_container) ->
    # Cache the jQuery object of the cats container
    $container = $(cats_container)

    # When the user scrolls
    $WIN.scroll ->
      # If we're not already loading new cats and we've passed the
      # threshold when we should grab start grabbing new cats, because
      # the user is almost near the bottom of their current cats...
      if not loading_new_cats and passed_pixels_from_bottom_threshold(PIXELS_FROM_BOTTOM_TO_PULL_NEW_CATS)
        # Set loading_new_cats to true to prevent more cats from being
        # brought in while we're still waiting for a batch of cats to load
        loading_new_cats = true
        # Append a certain number of cat iamges to our cats container
        append_cats NUMBER_OF_CATS_TO_APPEND
        # When all the newly appended cat images are loaded, allow for
        # new cats to be pulled in when the user nears the bottom
        # of the page again
        $(".#{IMG_CLASS}").load ->
          loading_new_cats = false

  # Tests whether we're a certain number of pixels from the bottom of the page

  passed_pixels_from_bottom_threshold = (pixels) ->
    scroll_distance = $WIN.scrollTop() + $WIN.height()
    document_length = $DOC.height()
    scroll_distance >= document_length - pixels

  # Appends a number of cats from the Cat API to our cats container

  append_cats = (number_of_cats) ->
    for [1..number_of_cats]
      random_cat = "#{CATS_ENDPOINT}&#{Math.random()}"
      $container.append """
        <img src=#{random_cat}" class="#{IMG_CLASS}" alt="Cat pic"></img>
      """

# When the page is completely loaded
$ ->
  # Activate a new InfiniteCatGenerator for the "#cats" container
  new InfiniteCatGenerator('#cats')

I also updated the HTML to put all the cats in a #cats container, which I like better than #bottomDiv, because it's more descriptive.

<div id="cats">
  <% @cats.each do |cat| %>
    <img src="<%= cat.url %>" class="cat-picture" alt="Cat pic">
  <% end %>
</div>
jlew715 commented 9 years ago

Should all be good now!

I know you’ve mentioned the camelCase thing before, I’m just very used to it from years of Java, Python, JavaScript, etc… I’ll keep it in mind for the future.

The whole using a random number to change the URL so the browser doesn’t cache the first one and use it repeatedly seems like such a workaround. Is this truly the best way to do that? I couldn’t find anything online. Well, it works, so I’ll probably leave it, but I’m just curious at this point.

Thanks!

On Apr 15, 2015, at 11:35 AM, Chris Fritz notifications@github.com wrote:

And one other piece of feedback. This isn't something you have to update, but I did a quick refactor of your code in CoffeeScript, to give you a taste of the power of CoffeeScript and also help you learn how to implement a slightly more powerful infinite scroll.

Don't hesitate to let me know if you have any questions/comments.

CoffeeScript classes rock for modularizing our code!

class InfiniteCatGenerator

---------

CONSTANTS

---------

These are constants are unlikely to ever change.

$WIN = $(window) $DOC = $(document)

----------------------

CONGIGURABLE CONSTANTS

----------------------

These are constants are likely to change as we decide

to tweak the behavior of the infinite scrolling.

IMG_CLASS = 'cat-picture' NUMBER_OF_CATS_TO_APPEND = 5 PIXELS_FROM_BOTTOM_TO_PULL_NEW_CATS = 500 CATS_ENDPOINT = 'http://thecatapi.com/api/images/get?format=src'

----------------

GLOBAL VARIABLES

----------------

These are variables that should be accessible in all our functions

$container = undefined loading_new_cats = false

The constructor function is automatically run whenever a new

InfiniteCatGenerator is created

constructor: (cats_container) ->

Cache the jQuery object of the cats container

$container = $(cats_container)
# When the user scrolls
$WIN.scroll ->
  # If we're not already loading new cats and we've passed the
  # threshold when we should grab start grabbing new cats, because
  # the user is almost near the bottom of their current cats...
  if not loading_new_cats and passed_pixels_from_bottom_threshold(PIXELS_FROM_BOTTOM_TO_PULL_NEW_CATS)
    # Set loading_new_cats to true to prevent more cats from being
    # brought in while we're still waiting for a batch of cats to load
    loading_new_cats = true
    # Append a certain number of cat iamges to our cats container
    append_cats NUMBER_OF_CATS_TO_APPEND
    # When all the newly appended cat images are loaded, allow for
    # new cats to be pulled in when the user nears the bottom
    # of the page again
    $(".#{IMG_CLASS}").load ->
      loading_new_cats = false

Tests whether we're a certain number of pixels from the bottom of the page

passed_pixels_from_bottom_threshold = (pixels) -> scroll_distance = $WIN.scrollTop() + $WIN.height() document_length = $DOC.height() scroll_distance >= document_length - pixels

Appends a number of cats from the Cat API to our cats container

append_cats = (number_of_cats) -> for [1..number_of_cats] random_cat = "#{CATS_ENDPOINT}&#{Math.random()}" $container.append """ <img src=#{random_cat}" class="#{IMG_CLASS}" alt="Cat pic"> """

When the page is completely loaded

$ ->

Activate a new InfiniteCatGenerator for the "#cats" container

new InfiniteCatGenerator('#cats') I also updated the HTML to put all the cats in a #cats container, which I like better than #bottomDiv, because it's more descriptive.

<% @cats.each do |cat| %> Cat pic <% end %>

— Reply to this email directly or view it on GitHub https://github.com/jlew715/tc359_rails_final/issues/4#issuecomment-93452277.

chrisvfritz commented 9 years ago

Since it is technically possible to get a random number twice in a row, a better solution might be to replace Math.random() with (new Date()).getTime(), which would always be different. The former does run about 17 times as fast as the latter, but for the number of times we're running that line, it could be argued that 4.5 million operations per second is still good enough. :smiley:

That doesn't keep the whole solution from feeling hacky though. An alternative would be to remove the src and set it again for the image, but that would only work if the Cat API has the header for that endpoint set so that:

<META HTTP-EQUIV="CACHE-CONTROL" CONTENT="NO-CACHE">

Is that any less hacky? Probably not. Unfortunately, there probably isn't a non-hacky solution here.