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

Fix(#10210): Hide pager when there is only one page (#10210) #10221

Merged

Conversation

SharkyBytes
Copy link
Contributor

Closes #10210

Fix: Hides the pager when there is only one page in the legacy-datatables UI. This prevents unnecessary pagination buttons that serve no purpose for users.

Technical

Testing

  1. Navigate to any book and go to the "View Editions" tab.
  2. If the allowed entries exceed the available data (e.g., only 8 entries), verify that the pager is hidden.
  3. If the dataset exceeds the allowed entries, verify that the pager is displayed as expected.

Screenshot

Pager Hidden (Single Page):
The pager is hidden because the allowed entries are more than the actual dataset (8 entries).
image
Pager Visible (Multiple Pages):
The pager is shown when the dataset exceeds the allowed entries.
image

Stakeholders

@RayBB

@SharkyBytes
Copy link
Contributor Author

Hi @RayBB & @mekarpeles,

I’ve implemented the fix to hide the pager when there’s only one page. Let me know if any changes are needed or if it’s good to merge.

On a related note, I noticed that other pages like Trending hide the "First/Previous" buttons on the first page and the "Next/Last" buttons on the last page. Would you like me to include this behavior here, or should we address it in a separate issue?

Trending Page Behaviour

image

image

For your reference, I’ve already run the tests locally using:

docker compose run --rm home make test

->All tests passed successfully, you can see below

image

Thanks again for your guidance and support @RayBB —it’s been great working with you!

@SharkyBytes SharkyBytes changed the title Fix(#10210): Hide pager in legacy datatables when only one page Fix(#10210): Hide pager when there is only one page (#10210) Dec 29, 2024
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Great work on the feature—it's coming along nicely! I noticed the pager is being hidden using CSS. While that works technically, it might not be the easiest approach for someone looking at the HTML to quickly understand what's going on.

For simplicity and clarity, it’s usually better to handle visibility directly in the HTML whenever possible. This way, if someone is searching for a specific element, they can find all the relevant details in one place without needing to cross-reference the CSS.

Would it be feasible to update this so the pager is hidden within the HTML itself? This small change could make the code easier to maintain and understand for future developers.

@SharkyBytes
Copy link
Contributor Author

Hi @RayBB,

Thanks for your feedback!

I’ve looked for the HTML file where the changes could be applied, but the updates don’t seem to reflect there. It appears the pager behavior is being controlled elsewhere, and I’m not seeing the expected result in the HTML file I modified.

As this is a large codebase, I’m still familiarizing myself with all the intricacies, so it would be really helpful if you could point me to the exact HTML file or location where these changes should be made.

@RayBB
Copy link
Collaborator

RayBB commented Dec 29, 2024

@SharkyBytes please check in /openlibrary/plugins/openlibrary/js/editions-table/index.js

@SharkyBytes SharkyBytes force-pushed the 10210/fix/hide-useless-pager branch from 151db16 to 1ccea97 Compare December 29, 2024 21:28
@SharkyBytes
Copy link
Contributor Author

I was also looking at the /openlibrary/plugins/openlibrary/js/editions-table/index.js file, but I initially thought there might be a dedicated HTML file for the pager behavior, similar to the other pagers we have in the codebase.

@RayBB, Please take a look at the changes I made, and let me know if this approach works or if you'd prefer a different solution.

@SharkyBytes SharkyBytes requested a review from RayBB December 29, 2024 21:39
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Let's try using the proper JavaScript API instead of adding new css.

@mekarpeles
Copy link
Member

This is where we deal with pagers in html https://github.com/internetarchive/openlibrary/blob/master/openlibrary/macros/Pager.html#L4 (this could be if pages > 1)

Within js for the editions table, here you can see if/when we are rendering the DataTable with bPaginate which is currently only if (rowCount < 4), but these conditions may not be sufficient.

rowCount = $('#editions tbody tr').length;
if (rowCount < 4) {
$('#editions').DataTable({
aoColumns: [{sType: 'html'},null],
order: [ [1,'asc'] ],
bPaginate: false,
bInfo: false,
bFilter: false,
bStateSave: false,
bAutoWidth: false
});
} else {
currentLength = Number(localStorage.getItem(LS_RESULTS_LENGTH_KEY));
$('#editions').DataTable({
aoColumns: [{sType: 'html'},null],
order: [ [1,'asc'] ],
lengthMenu: [ [3, 10, 25, 50, 100, -1], [3, 10, 25, 50, 100, 'All'] ],
bPaginate: true,
bInfo: true,
sPaginationType: 'full_numbers',
bFilter: true,
bStateSave: false,
bAutoWidth: false,
pageLength: currentLength ? currentLength : DEFAULT_LENGTH
});

@mekarpeles mekarpeles added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 29, 2024
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Dec 30, 2024
@SharkyBytes
Copy link
Contributor Author

SharkyBytes commented Dec 30, 2024

Hey @RayBB & @mekarpeles ,

I've updated the code to dynamically handle the pagination visibility using the DataTables API. Now, the pagination controls are hidden when the total row count is less than or equal to the selected page length or when 'All' is selected in the dropdown. Let me know if there's anything else you'd like me to tweak!

After Changes-
image
image

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

Looks like a step in the right direction!
Can you please remove all the whitespace changes you've made so it's easy to see the needed changes? You might need to disable automatic code formatting in your IDE if it's enabled.
Also there are now failing checks. Please address these (they're small ones).

Thanks for your patience. I know these small things can be a pain.

@SharkyBytes SharkyBytes force-pushed the 10210/fix/hide-useless-pager branch from 9c998e6 to b06f2c7 Compare December 30, 2024 10:40
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.

Project coverage is 17.42%. Comparing base (3025f5d) to head (830e934).
Report is 71 commits behind head on master.

Files with missing lines Patch % Lines
...ary/plugins/openlibrary/js/editions-table/index.js 0.00% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10221      +/-   ##
==========================================
- Coverage   17.44%   17.42%   -0.02%     
==========================================
  Files          89       89              
  Lines        4792     4797       +5     
  Branches      848      849       +1     
==========================================
  Hits          836      836              
- Misses       3436     3439       +3     
- Partials      520      522       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SharkyBytes
Copy link
Contributor Author

Hey @RayBB and @mekarpeles ,

Just a quick update — I’ve improved the logic and cleaned up the auto-formatting changes so the code looks cleaner now. All checks have passed, so it’s ready for review. Let me know if you spot anything.

@SharkyBytes SharkyBytes requested a review from RayBB December 30, 2024 10:46
Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

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

This seems like a fantastic solution to me! Great job 👍

Hiding the element isn't ideal but I don't think there's a better way to do it.

Now we just need staff to review/merge.

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label Dec 30, 2024
@mekarpeles
Copy link
Member

I believe this change set now LGTM. 👍 thank you!

@mekarpeles mekarpeles merged commit ab74711 into internetarchive:master Dec 30, 2024
4 checks passed
@SharkyBytes
Copy link
Contributor Author

Thanks for merging @RayBB & @mekarpeles! Really loved working on this with you both. Appreciate your support and feedback throughout :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Stop Showing Pagination Buttons When There is only one page
4 participants