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

WV-861 add next page UI [TEAM REVIEW] #2821

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

itcreativeusa
Copy link
Contributor

1)Implemented backend logic to support pagination with total_count, items_per_page, start_index, and end_index.
2) Added previous_page_url and next_page_url for navigation.
3) Conditionally hid pagination UI when items fit on a single page.
4)Updated the frontend to display "Previous" and "Next" links with the current page number.

@itcreativeusa itcreativeusa force-pushed the WV-861-add-next-page-ui branch from 37806d0 to daddbd9 Compare December 14, 2024 06:06
@DaleMcGrew DaleMcGrew changed the title Wv 861 add next page UI WV-861 add next page UI [TEAM REVIEW] Dec 17, 2024
[WV-861]Add 'Next Page' UI on Politician Admin page
@itcreativeusa itcreativeusa force-pushed the WV-861-add-next-page-ui branch from daddbd9 to de1f17e Compare December 29, 2024 02:47
Copy link
Member

@DaleMcGrew DaleMcGrew left a comment

Choose a reason for hiding this comment

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

Hi Katerenya, overall you are on the right track. I have these comments for your next iteration. Thank you!

@@ -336,5 +367,6 @@ <h1>Politicians</h1>
});

</script>
<script src="https://code.jquery.com/jquery-3.6.0.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Hi Katerenya, we shouldn't have to include this here because jquery is already included in WeVoteServer/templates/template_base.html on line 24. You can upgrade that version in that file if you need a newer version.

template_values = {
"start_index": start_index,
'politician_list': politician_list,
Copy link
Member

Choose a reason for hiding this comment

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

Please add these template variables in alphabetical order, because that helps us make sure we don't duplicate existing variables. (I see "politician_list" and "politician_search" for example are already passed in further down in this template_values dict.)

end_index = start_index + items_per_page

# Fetch only the items for the current page
politician_list = politician_query.order_by("politician_name")[start_index:end_index]
Copy link
Member

Choose a reason for hiding this comment

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

We have already retrieved politician_list above, and augmented it with data from the Representative table. This replaces that work with a brand new query. I think this pagination logic should be included up around lines 992-996 where the current retrieve is happening:

    if not positive_value_exists(show_all):
        politician_list = politician_query.order_by('politician_name')[:25]
    else:
        # We still want to limit to 200
        politician_list = politician_query.order_by('politician_name')[:200]

@@ -51,22 +51,24 @@ <h1>Politicians</h1>

<form name="state_code_form" method="get" action="{% url 'politician:politician_list' %}">
{% csrf_token %}
<input type="hidden" name="politician_search" value="{{ politician_search|default_if_none:'' }}" />
Copy link
Member

Choose a reason for hiding this comment

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

politician_search is already being included in the form down below on your line 74 in an input, and doesn't need to be added here as a hidden value.

@@ -51,22 +51,24 @@ <h1>Politicians</h1>

<form name="state_code_form" method="get" action="{% url 'politician:politician_list' %}">
{% csrf_token %}
<input type="hidden" name="politician_search" value="{{ politician_search|default_if_none:'' }}" />
<input type="hidden" name="show_all" value="{{ show_all|default_if_none:0 }}" />
Copy link
Member

Choose a reason for hiding this comment

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

show_all is already included below as a checkbox, and doesn't need to be included here as a hidden value.

@@ -51,22 +51,24 @@ <h1>Politicians</h1>

<form name="state_code_form" method="get" action="{% url 'politician:politician_list' %}">
{% csrf_token %}
<input type="hidden" name="politician_search" value="{{ politician_search|default_if_none:'' }}" />
<input type="hidden" name="show_all" value="{{ show_all|default_if_none:0 }}" />
<input type="hidden" name="exclude_politician_analysis_done" value="{{ exclude_politician_analysis_done|default_if_none:0 }}" />
Copy link
Member

Choose a reason for hiding this comment

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

exclude_politician_analysis_done is already included below as a checkbox, and doesn't need to be included here as a hidden value.

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