Update-JavaScript-for-search-pagination ( Continuation after PR #9224 ) #9941
Conversation
Updated Query Selectors to use Data attributes instead of Class or Id names Also Removed Id tags for buttons
Manage this branch in SquashTest this branch here: https://aym-nupdate-javascript-for-sea-j48jf.squash.io |
|
|
@lb- CI backend test is failing and the log output is |
aym-n
changed the title
|
couldn't find out what is causing the UI Tests Error |
|
I think the failing Axe test is a known issue. #8938 |
|
@lb- Thanks for reviewing my PR! What can I do to continue contributing to this issue? |
|
@aym-n this hasn't been reviewed yet, but we will tag it for review. Not sure on next steps at this time but myself or someone else will review and get back to you. Thank you for taking the time to get this PR together. |
There was a problem hiding this comment.
Thank you for giving this a go @aym-n! Looks like you’ve made a lot of improvements from this previous pull request, but there are still a few dealbreaker issues (see individual comments).
I think it’s also problematic we’re now combining the separate "search" and "pagination" code all in the same file. The pagination code wasn’t designed to be imported on all pages of the site, so I think it would need even bigger refactorings for that to work.
Perhaps we should rename search.js to pagination.js, keep the layout.html template as-is, and load this pagination.js on the search.html template only?
| * See https://github.com/algolia/docsearch-configs/blob/master/configs/wagtail.json for index configuration. | ||
| */ | ||
| function docSearchReady() { | ||
| try { |
There was a problem hiding this comment.
The fact we now have to wrap this code in a try…catch looks like an anti-pattern to me. Can we deal with the errors that are arising here another way?
| // Display pagination if more than 1 page returned | ||
| const { nbPages } = result; | ||
| if (nbPages > 1) { | ||
| document.querySelector('#pagination').hidden = false; |
There was a problem hiding this comment.
This can’t work because we’ve removed id="pagination" from that element. There’s lots of other code below which also relies on this id, so I’d assume the pagination is completely broken as-is.
| for (let i = start; i < end; i += 1) { | ||
| const newPaginationItem = document.createElement('li'); | ||
| const newPaginationbutton = document.createElement('button'); | ||
| newPaginationbutton.classList.add('pagination-button'); |
There was a problem hiding this comment.
This button isn’t styled correctly in our dark theme for the documentation.
thibaudcolas
added
status:Needs Review
status:Needs Work
and removed
status:Needs Review
labels
See #9102 – based upon #9224.