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

Create pagination for the index page #198 #201

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

Conversation

IlinDmitry
Copy link

Hello.
Nice issue for newby web programmers like me.

Copy link
Member

@davydovanton davydovanton left a comment

Choose a reason for hiding this comment

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

Sorry for the delay.

First of all: thanks for this great contribution 👍
Also, could you add some screenshots before merge and also fix style issues?

@@ -29,5 +31,9 @@ def with_language(search_params)
def status
Task::VALID_STATUSES.values.include?(params[:status]) ? params[:status] : 'in progress'
end

def limit
10
Copy link
Member

Choose a reason for hiding this comment

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

let's make limit as 50?

expose :tasks

def call(params)
@tasks = TaskRepository.new.find_by(search_params)
repo = TaskRepository.new.find_by(search_params)
@tasks = all_for_page(repo)
Copy link
Member

Choose a reason for hiding this comment

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

it's not a repo 🤔
let's rename it to relation

@@ -76,5 +77,85 @@ def author_information(author, task)
text(" • #{task.lang}")
end
end

def pagination
html.div(class: 'pagination') do
Copy link
Member

Choose a reason for hiding this comment

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

we really need all this code without any specs here?

Copy link
Author

Choose a reason for hiding this comment

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

I wrote this code, because pagination function was not available in hanami-pagination gem.
Now i replaced it with gem function.

@@ -29,7 +29,7 @@ def all_from_date_counted_by_status_and_day(from)
end

def find_by(params = {})
tasks.where(params).order { id.desc }.map_to(Task).to_a
tasks.where(params).order { id.desc }.map_to(Task)
Copy link
Member

Choose a reason for hiding this comment

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

WDYT if we split this code into two methods:

def find_by(params = {})
  search_relation(params).to_a
end

def search_relation(params)
  tasks.where(params).order { id.desc }.map_to(Task)
end

end

it 'returns 10 tasks on page' do

Copy link
Member

Choose a reason for hiding this comment

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

missing space here

@@ -155,6 +155,41 @@
expect(action.tasks.map(&:status)).to all(eq('in progress'))
end
end

context 'when are on the first page' do
Copy link
Member

Choose a reason for hiding this comment

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

let's simplify specs here. For this we can use DI (dependency injection and wiki) for repository instance:

So, I found a problem in pagination davydovanton/hanami-pagination#10, thanks for this! 💯 🌟 🎉


end


Copy link
Member

Choose a reason for hiding this comment

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

could you cut unnecessary spaces here and format specs? thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Ok, i will focus on it more.

@IlinDmitry
Copy link
Author

Hi.
Could you tell me please, what kind of style issues? i don't see any problem with css.
2page
1page

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.

2 participants