Stop using jquery simple pagination #157
Conversation
productcomments.php
Outdated
| $commentsTotalPages = ceil($commentsNb / $commentsPerPage); | ||
| $commentsNav .= '<ul>'; | ||
| $prevCount = 0; | ||
| $commentsNav .= '<li data-page="' . $prevCount . '" id="pcl_page_' . $prevCount . '"><span class="prev"><i class="material-icons">chevron_left</i></span></li>'; |
There was a problem hiding this comment.
maybe it would be better to have a dedicated template for $commentsNav rather than writing serious HTML in PHP?
There was a problem hiding this comment.
Done. This pure HTML is moved to TPL.
views/js/list-comments.js
Outdated
| let i = start + step; | ||
| if (4 < Math.abs(stop - start)) { | ||
| $(pageIdPrefix + i).removeClass('hidden').removeClass('disabled'); | ||
| $(pageIdPrefix + i + ' span').html(i); |
There was a problem hiding this comment.
Consider using template literals instead of contatenating with + (here and in other selectors when applicable)
| $(pageIdPrefix + i + ' span').html(i); | |
| $(`${pageIdPrefix}${i} span`).html(i); |
There was a problem hiding this comment.
Applied. We have a bunch of $$$ now 🏦
|
LGTM! One thing I need clarification on, to me, it looks like a BC Break. Imagine this. If somebody updates the module and has an override on the list comments, it won't work anymore. The same goes for the view file. What do you think? |
|
@kpodemski I agree current changes in views/templates/hook/product-comments-list.tpl is a BC break in case a theme overrided |
|
After rechecking views/js/list-comments.js I'm quite sure that changes in views/templates/hook/product-comments-list.tpl is not a BC break as it looks like. Because whatever a theme developer wrote in the theme |
There was a problem hiding this comment.
Hello @leemyongpakvn ,
Thanks for your PR !
LGTM, it is QA ✅
Screen.Recording.2023-03-31.at.14.46.01.mov
|
well done @leemyongpakvn 👏🏻 I see more and more PRs from you, and more complicated ones, I hope we can have you as a committer soon :D |
|
@kpodemski I'm simplifying complex things ;) Becoming a committer can make my contributions and some other contributors ones move faster but I'm worried about something that @matks's scared of. The less active status of the 2 safest-looking maintainers (according to my point of view :) and the increasing concern of matks about Instances of abusive and harassing in recent weeks even make me more worried 😟 |
It is a chance to replace old jquery get by new fetch API too.
(other contributors can try to replace jquery post by fetch API or even remove unnecessary JS code :)
There is a behavior change for comment list with over 9 pages when click
...sign: old pagination show an input box to enter page number, while new pagination reveal the hidden page number instead.