Documentation - pagination improvements

[lb-]

Is your proposal related to a problem?

• Pagination for docs was added via Added pagination to docs search results page #8178 as a solution for Documentation search: pagination for results page #6886
• In that review there were a few extra improvements flagged that would be good to resolve.

This is a good first issue for someone wanting to work with JavaScript and the documentation code.

Describe the solution you'd like

• Move JS to a separate file instead of adding scripts inline in https://github.com/wagtail/wagtail/blob/main/docs/_templates/search.html
• Display either one button per page, or just prev/next with a count of "Page 3 of 10".
• Replace usage of onclick with addEventListener - this is preferred

Additional context

• read https://developer.mozilla.org/en-US/docs/Web/Events/Event_handlers#eventtarget.addeventlistener for context on why addEventListener is preferred
• Originally posted by @thibaudcolas in Added pagination to docs search results page #8178 (review)

[lb-]

Note - this was flagged as part of the UX Unification (GSoC 2022) project - but de-scoped.

Labelling as a good first issue though.

[akshay-gupta7]

Hi @lb- , can you assign this to me

[lb-]

@akshay-gupta7 we do not really assign issues to contributors, only the core team in some cases.

Feel free to get started with this and prepare a PR when you are ready.

Reach out if you need a hand, and remember there is a #new-contributors channel on Slack.

[akshay-gupta7]

@lb- sure, thank you. I'll get started with this.

[akshay-gupta7]

Hi @lb- , raised a PR against this issue. Please do have a look whenever you can

[akshay-gupta7]

Hi @lb- , thank you for taking out the time to review my code. As per your review, I have put in the suggestions. Please do have a look whenever you can.

[lb-]

I have put some feedback on the PR but just adding some solution ideas here, but the general gist is that we probably should tackle this in two parts.

A. clean up the existing code (use proper event listeners, move the code to an external file search.js and solve any large linting issues.
B. Adopt a new behaviour/set of buttons for the pagination.

For item B, we need to rethink the JS solution which iterates through the 'pages' and adds buttons for each page, instead it can be much simpler and just have TWO buttons (prev/next) and one status label (Page X of Y) and avoid lots of the need for aria-current.

Here is a rough potential base HTML for the template, leveraging data attributes to attach our JS to and places to attach the X/Y labels to. Note: This may need a full review in a screen reader and accessibility checker also.

{% extends "layout.html" %}
{% set title = _('Search') %}

{% block body %}
    <noscript>
        <div id="fallback" class="admonition warning">
            <p class="last">
                {% trans trimmed %}
                    Please activate JavaScript to enable the search functionality.
                {% endtrans %}
            </p>
        </div>
    </noscript>

    <div id="search-results">
    </div>

    <nav class="pagination" aria-label="Pagination" hidden="true" data-pagination="7">
        <button type="button" class="pagination-button pagination-previous" data-pagination-previous aria-describedby="pagination-status">
            {% trans trimmed %}← Previous{% endtrans %}
        </button>
        <span id="pagination-status">
            {% trans trimmed %}Page <span data-pagination-status-x></span> of <span data-pagination-status-y></span>{% endtrans %}
        </span>
        <button type="button" class="pagination-button pagination-next" data-pagination-next aria-describedby="pagination-status">
            {% trans trimmed %}Next →{% endtrans %}
        </button>
    </nav>
{% endblock %}

Then in the JS, we can make things much simpler, and just attach to these elements. Something like the below as a start.

function displayPagination(page, totalPages) {
  const pagination = document.querySelector('[data-pagination]');
  const maxPagesToDisplay = Number(pagination.dataset.pagination) || 7;

  // Hide previous/next button if showing first/last page
  const previousButton = pagination.querySelector('[data-pagination-previous]');
  previousButton.hidden = page === 0;

  const nextButton = pagination.querySelector('[data-pagination-next]');
  nextButton.hidden = page === totalPages - 1;
  
  // update status labels
  const xLabel = pagination.querySelector('[data-pagination-status-x]');
  xLabel.innerText = `${page}`;

  const yLabel = pagination.querySelector('[data-pagination-status-y]');
  yLabel.innerText = `${Math.min(totalPages, maxPagesToDisplay)}`;

 // add event listeners here to the prev/next button

This approach avoids a lot of the JS writing of content (Page X of Y) and keeps that in the initial DOM, and hopefully we can avoid the complexity of iteration.

This may not cover the full solution but just some ideas on approaches.

Also, we need to consider the following when adopting this new code.

• Browser support, esp. Safari ~14+ & mobile devices
• Dark mode support (does everything look good in dark mode and non-dark mode)
• JS does not have errors or console warnings at all, including pages that do not use the search results
• Code is commented where possible and JSDOC is used to help document this as it is a bit complex
• Event listeners are removed when needed - or at least the code, if run, will not do anything odd when pages change
• All buttons have type="button" so they do not cause a page reload https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button
• All aria-current attributes are removed IF we deem they are no longer required for the new approach https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-current
• All aria attributes make sense, and where possible, we use semantic DOM elements instead (e.g. we may not need the aria-label on the outer container if we just used a proper header element for that nav element).

All of this means that the change of the button layout is probably a larger task on its own so any PRs are welcome that make partial progress towards this overall goal.

Any feedback is welcome on this rough code also.

cc @akshay-gupta7

[aym-n]

@lb- can you assign me this issue? i would like to work on it

[lb-]

@aym-n there is a PR already started #9224 and also some direction above. Please review it all and if you want to give this a go as a new PR - go for it.

@akshay-gupta7 - it's been a few months so I'm assuming you are ok for someone else to give this a go. Let me know otherwise.

[aym-n]

• clean up the existing code (use proper event listeners, move the code to an external file search.js and solve any large linting issues.
• we need to rethink the JS solution

[aym-n]

working on cleaning up the code and the tasks mentioned in the #9224

[lb-]

@aym-n just a tip - when you are referencing an issue you just need the hash in front of it, not the PR.

Observe that PR#9224 does not make a link but #9224 does.

See https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/autolinked-references-and-urls

[aym-n]

Observe that PR#9224 does not make a link but #9224 does.

Thank You!
Will keep that in mind from now on :)

[MashiatK]

I want to work on this. If the issue is open, please assign it to me.

[gasman]

@MashiatK Please read the discussion above - there is already a pull request for this.