Create Paginator Module #60

ghost · 2017-03-27T17:39:28Z

Pivotal: https://www.pivotaltracker.com/story/show/140512959
Paginator Module displays X number of Model items per page and outputs a corresponding pagination panel.

jacquesporveau · 2017-03-29T22:08:32Z

app/views/shared/_paginator.html.erb

+    <% blocks.each do |page|%>
+      <%= render 'shared/blocks', page: page,
+        page_item_css: page_item_css(page,
+                                     current_page) %>


This is kind of weird code style. You've decided to put 2 arguments on the first line and one on the second.

Thanks, just noticed!

jacquesporveau · 2017-03-29T22:10:07Z

lib/utilities/paginator.rb

+  module Paginator
+    def items_to_display(model:, items_per_page: 5, current_page: 1)
+      @items_per_page = items_per_page
+      @current_page = current_page.to_i


Why are we calling to_i on current page? I would assume we are passing in a integer to this method and not a float or string.

Ah yeah, it's because in my BetsController I pass the parameter current_page: params[:page] as the argument. So, in this case it's either I apply it here just to be safe and make sure it's always an int or I can pass it there.
Please let me know what works best.

Yeah I'd like to see that done in the controller. To have current page referring to a string or integer depending on where it is being used is odd. Also is there an instance we need it to be a string? Usually the string "1" is pretty useless.

jacquesporveau · 2017-03-29T22:13:30Z

lib/utilities/paginator.rb

+      elsif current_page_at_the_beginning?(current_page, max_page_blocks)
+        beginning_pagination(max_page_blocks)
+      elsif current_page_at_the_end?(current_page,
+                                     number_of_pages, max_page_blocks)


One argument on the first line and two on the second.

I remember why I did it now - Rubocop won't let me have 11 lines of code, so. I shuffled it around - 2 lines on top, one at the bottom, hope it looks better.

Maybe just one argument per line and then we can be consistent.

jacquesporveau · 2017-03-29T22:14:38Z

spec/modules/paginator_spec.rb

+require 'rails_helper'
+
+RSpec.describe Utilities::Paginator, type: :module do
+  let!(:dummy) { Class.new { extend Utilities::Paginator } }


Perhaps this could be called 'paginator'?

natyork · 2017-04-04T23:07:34Z

lib/utilities/paginator.rb

+                       last_page: @last_page }
+    end
+
+    def page_item_css(page, current_page)


Perhaps a more semantic name for this method would be something like page_item_class_name?

natyork · 2017-04-04T23:13:38Z

lib/utilities/paginator.rb

+    def page_item_css(page, current_page)
+      if page == current_page
+        'page-item active'
+      elsif page.to_s == '...'


You use the ellipses (...) in several places - it might be worth making it a constant, in case you ever want to change it in the future. This will also allow you to give it a more descriptive/semantic name so that you don't have 'Magic Numbers' (or text in this case) in your code.

natyork · 2017-04-04T23:16:11Z

lib/utilities/paginator.rb

+    private
+
+    def previous_link_css
+      @current_page == 1 ? 'page-item disabled' : 'page-item'


Magic number - this could be made a constant such as FIRST_PAGE instead of 1.

natyork · 2017-04-04T23:18:15Z

lib/utilities/paginator.rb

+    end
+
+    def current_page_at_the_beginning?(current_page, max_page_blocks)
+      current_page <= (max_page_blocks / 2)


magic number

natyork · 2017-04-04T23:18:44Z

lib/utilities/paginator.rb

+
+    def current_page_at_the_end?(current_page, number_of_pages, max_page_blocks)
+      current_page > (number_of_pages - (max_page_blocks / 2))
+    end


magic number

natyork · 2017-04-04T23:19:02Z

lib/utilities/paginator.rb

+      else
+        even_pages(current_page, max_page_blocks)
+      end
+    end


magic numbers

natyork · 2017-04-04T23:19:19Z

lib/utilities/paginator.rb

+      start = 1
+      finish = max_page_blocks - 1
+      [*(start..finish), '...']
+    end


magic numbers

natyork · 2017-04-04T23:19:35Z

lib/utilities/paginator.rb

+      start = number_of_pages - max_page_blocks + 2
+      finish = number_of_pages
+      ['...', *(start..finish)]
+    end


magic number

natyork · 2017-04-04T23:19:50Z

lib/utilities/paginator.rb

+
+    def complete_pagination(last_page)
+      [*1..last_page]
+    end


magic number

natyork · 2017-04-04T23:19:57Z

lib/utilities/paginator.rb

+      start = (current_page - (max_page_blocks - 1) / 2) + 1
+      finish = (current_page + (max_page_blocks - 1) / 2) - 1
+      ['...', *(start..finish), '...']
+    end


magic numbers

natyork · 2017-04-04T23:20:10Z

lib/utilities/paginator.rb

+      start = (current_page - (max_page_blocks - 1) / 2) + 1
+      finish = (current_page + (max_page_blocks / 2)) - 1
+      ['...', *(start..finish), '...']
+    end


magic numbers

natyork

Overall looks pretty good, but I've flagged areas where you are using so-called 'magic numbers'. It would make your code much more readable and maintainable to use semantically-named constants rather than raw numbers/text that require interpretation throughout your code.

Allow long strings for "context"

Adding pagination module that will have to be included in autoload_paths.

-Paginator module is responsible for calculating and rendering a pagination bar based on the passed parameters. -Added rspec for Paginator.

-Add _blocks.html.erb view that renders page blocks; -Add _paginator.html.erb view that renders the pagination panel for the current page of a given Model.

ghost mentioned this pull request Mar 27, 2017

Add view button for every displayed Bet in /bets #37

Merged

ghost force-pushed the view_bets_and_pagination_commits branch from 7701211 to 405cde9 Compare March 27, 2017 23:16

ghost changed the title ~~Users can see all the bets~~ Create Paginator Module Mar 27, 2017

ghost mentioned this pull request Mar 27, 2017

Add Paginator to Bet #61

Open

ghost force-pushed the view_bets_and_pagination_commits branch 2 times, most recently from 5db032c to 369fa14 Compare March 29, 2017 18:49

jacquesporveau reviewed Mar 29, 2017

View reviewed changes

ghost force-pushed the view_bets_and_pagination_commits branch 2 times, most recently from 40f6a74 to d3ffda1 Compare March 30, 2017 16:10

natyork reviewed Apr 4, 2017

View reviewed changes

lib/utilities/paginator.rb

else

even_pages(current_page, max_page_blocks)

end

end

Copy link

natyork Apr 4, 2017

Choose a reason for hiding this comment

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

magic numbers

natyork reviewed Apr 4, 2017

View reviewed changes

lib/utilities/paginator.rb Outdated

start = 1

finish = max_page_blocks - 1

[*(start..finish), '...']

end

Copy link

natyork Apr 4, 2017

Choose a reason for hiding this comment

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

magic numbers

natyork reviewed Apr 4, 2017

View reviewed changes

lib/utilities/paginator.rb Outdated

def complete_pagination(last_page)

[*1..last_page]

end

Copy link

natyork Apr 4, 2017

Choose a reason for hiding this comment

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

magic number

natyork reviewed Apr 4, 2017

View reviewed changes

ghost force-pushed the view_bets_and_pagination_commits branch 5 times, most recently from ed870d5 to 23b4f2a Compare April 10, 2017 17:40

LeonTenko added 4 commits April 11, 2017 11:32

Update Rubocop

cceed6c

Allow long strings for "context"

Add /lib folder to autoload_paths

6a369e0

Adding pagination module that will have to be included in autoload_paths.

Create Paginator module

27d2a54

-Paginator module is responsible for calculating and rendering a pagination bar based on the passed parameters. -Added rspec for Paginator.

Add views for Paginator module

a9f2557

-Add _blocks.html.erb view that renders page blocks; -Add _paginator.html.erb view that renders the pagination panel for the current page of a given Model.

ghost force-pushed the view_bets_and_pagination_commits branch from 23b4f2a to a9f2557 Compare April 11, 2017 18:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Paginator Module #60

Create Paginator Module #60

ghost commented Mar 27, 2017 •

edited by ghost

Loading

jacquesporveau Mar 29, 2017

ghost Mar 29, 2017

jacquesporveau Mar 29, 2017

ghost Mar 29, 2017

jacquesporveau Mar 29, 2017

jacquesporveau Mar 29, 2017

ghost Mar 29, 2017

jacquesporveau Mar 29, 2017

jacquesporveau Mar 29, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork Apr 4, 2017

natyork left a comment

Create Paginator Module #60

Are you sure you want to change the base?

Create Paginator Module #60

Conversation

ghost commented Mar 27, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natyork left a comment

Choose a reason for hiding this comment

ghost commented Mar 27, 2017 •

edited by ghost

Loading