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

pagination and navigation #1974

Merged
merged 28 commits into from
May 23, 2019
Merged

pagination and navigation #1974

merged 28 commits into from
May 23, 2019

Conversation

ayrat555
Copy link
Contributor

@ayrat555 ayrat555 commented May 17, 2019 •
edited
Loading

part of #1930

img

Changelog

  • add js logic to save a stack of visited pages and set the previous page to the previous button href
  • add page number that is calculated using the visited pages stack
  • set new logic to all pages with async data loading
  • hide UI elements with unimplemented functionality (the last page, the first page, the number of items dropdown

Next PR(s)

  • load data async on the addresses list page
  • add functionality for the first and the last page buttons
  • add functionality for the number of items dropdown

@ghost ghost assigned ayrat555 May 17, 2019
@ghost ghost added the in progress label May 17, 2019
@coveralls
Copy link

coveralls commented May 20, 2019 •
edited
Loading

Pull Request Test Coverage Report for Build 283edaec-986c-4a5a-a3c6-dda64cabcd9f

  • 0 of 4 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 81.168%
Changes Missing Coverage Covered Lines Changed/Added Lines %
apps/block_scout_web/lib/block_scout_web/controllers/pagination_helpers.ex 0 4 0.0%
Totals Coverage Status
Change from base Build d6ae1f6c-6a76-4b15-b508-0a4191e3e10a: 0.05%
Covered Lines: 4724
Relevant Lines: 5820
💛 - Coveralls

@ayrat555 ayrat555 changed the title [WIP] pagination and navigation pagination and navigation May 21, 2019
@ayrat555 ayrat555 marked this pull request as ready for review May 21, 2019 11:29
@ayrat555 ayrat555 requested a review from pasqu4le May 21, 2019 13:08
zachdaniel
zachdaniel reviewed May 21, 2019
View reviewed changes
...ock_scout_web/lib/block_scout_web/templates/common_components/_pagination_container.html.eex
@@ -1,5 +1,5 @@
<div class='pagination-container <%= if assigns[:position] == "top" do %>position-top<% end %> <%= if assigns[:position] == "bottom" do %>position-bottom<% end %>'>
<%= if assigns[:show_pagination_limit] do %>
<%= if false do %>
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unintentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachdaniel no, this is intentional because the logic for choosing the number of items on the page is unimplemented yet

zachdaniel
zachdaniel approved these changes May 21, 2019
View reviewed changes
Copy link
Contributor

@zachdaniel zachdaniel left a comment

Choose a reason for hiding this comment

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

LGTM, aside from my comment about the if false

@ayrat555 ayrat555 requested a review from saneery May 22, 2019 07:28
saneery
saneery approved these changes May 22, 2019
View reviewed changes
Copy link
Contributor

@saneery saneery left a comment

Choose a reason for hiding this comment

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

Looks great!

gabitoesmiapodo
gabitoesmiapodo approved these changes May 22, 2019
View reviewed changes
Copy link
Contributor

@gabitoesmiapodo gabitoesmiapodo left a comment

Choose a reason for hiding this comment

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

Nice.

vbaranov
vbaranov reviewed May 22, 2019
View reviewed changes
Copy link
Member

@vbaranov vbaranov left a comment

Choose a reason for hiding this comment

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

@ayrat555 looks good

2 things I'd like to mention:

  1. Let's remove the loader, that is left from the previous design
    Screenshot 2019-05-22 at 18 43 48

  2. it could be done in the future updates of pagination. No necessary to include it in the current PR, but it would be nice to hide pagination view if there are no items

Screenshot 2019-05-22 at 18 51 28

@ayrat555
Copy link
Contributor Author

@vbaranov

  1. I removed the loader
  2. I'll add hiding in the next PR

@ayrat555 ayrat555 merged commit a55af3f into master May 23, 2019
@ayrat555 ayrat555 deleted the ab-pagination branch May 23, 2019 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vbaranov vbaranov vbaranov left review comments

@gabitoesmiapodo gabitoesmiapodo gabitoesmiapodo approved these changes

@zachdaniel zachdaniel zachdaniel approved these changes

@saneery saneery saneery approved these changes

@pasqu4le pasqu4le pasqu4le approved these changes

Assignees

@ayrat555 ayrat555

Labels
in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ayrat555 @coveralls @gabitoesmiapodo @vbaranov @zachdaniel @saneery @pasqu4le