UX
Great job on the end product! There are no distracting CSS animation frills and everything appears to work smoothly.
Consider disabling the forward/backward arrows on the first and last pages instead of removing them entirely. This way, the numbers are always in a fixed place and the user won't accidentally click the wrong thing or have to reposition their cursor after the page numbers shift unexpectedly after a click.
Use <strong>
instead of <b>
. HTML is for semantics/structure, not style/presentation.
Consider <button>
instead of <a>
for similar reasons. The <a href="#">
leaves an ugly #
in the browser URL (yeah, you can use javascript:void(0)
but that's also obtrusive). These buttons could be in a list.
Use const
instead of let
Instead of using let
everywhere, prefer using const
everywhere. Yes, it's two more characters to type but it has at least three benefits:
const
prevents a class of bugs related to inadvertent reassigments. Take any increase in guarantees you can get for free.
const
adds expressiveness to the code: when you do use let
, it communicates to the reader that you intend to reassign the variable, such as in a loop counter. If you apply let
across the board, a reader can't distinguish effectively-final variables (like pageNums
) and intended-to-be-reassigned dynamic variables (like currentPage
).
If you make an effort to prioritize const
throughout your code, yet you notice you're using let
excessively, it's a useful code smell indicating over-reliance on mutability. This applies to this program. Frequently, you declare some numerical variable with let
, then mutate it in a series of branches to clamp to max/min bounds and so forth. It's not obvious what sort of values they might have at a given program point.
If you challenge yourself to code without let
, you'd have to break this logic into functions. You could use ternaries, but ternaries are OK in my book as long as they're not nested. +=
can become map
/join
.
Refactoring let
into const
should automatically make your code safer and cleaner.
Style suggestions
.title>small
should be .title > small
so it doesn't look like a single class name.
I like the config object parameter for pagination
but par
isn't the clearest variable name. Consider destructuring par
into variables or rename it to config
.
"current-page"
in your pagination config is odd because kebab case isn't used in JS. Prefer currentPage
for easy destructuring and style consistency.
let list = data.list;
can use destructuring const {list} = data;
. You don't have to see list
twice and it's easier to add more properties in the future.
Spread long HTML properties onto multiple lines. Try to stick to a max width of 80-90 characters.
htmlText += `
<a class="${currentPage === i ? 'item-link active' : 'item-link'}" href="#" data-index="${i}" onClick="pagination({'current-page': ${i}, 'page-size': ${pageSize}})">
${i}
</a>
`;
becomes
htmlText += `
<a
class="${currentPage === i ? 'item-link active' : 'item-link'}"
href="#"
data-index="${i}"
onClick="pagination({'current-page': ${i}, 'page-size': ${pageSize}})"
>
${i}
</a>
`;
Functions should be verbs, classes should be nouns. pagination
should be called paginate
or createPagination
. If it were a class, it'd be Pagination
or Paginator
.
Avoid monolithic functions
pagination
is too large. Try to break it into helper functions for logical subtasks that build HTML or compute start/end bounds.
For example,
let totalPage = 0;
if (num / pageSize > parseInt(num / pageSize)) {
totalPage = parseInt(num / pageSize) + 1;
} else {
totalPage = parseInt(num / pageSize);
}
could be a named function. In addition to modularity, parameters and return
give a lot of power to avoid reassignments, mutability and ugly control flow.
Beyond that, parseInt(num / pageSize)
is repeated 3 times and num / pageSize
4 times. Math.floor
communicates what this code is doing better than parseInt
.
In this case, I think the whole thing can be replaced with const totalPages = Math.ceil(num / pageSize)
.
Keep Math.max
and Math.min
in mind to replace if
-else
blocks.
Avoid hardcoded globals
The pagination
function isn't reusable because it's tied to a special variable data
in the external scope. data
must have a list
property, every item in the list has to have title
and id
properties and document.querySelector(".pagination")
has to exist.
Your intent was probably to make a function so that it could be called as a handler -- basically, bare minimum to operate. This is fine for a toy example but not suitable for a large app.
If the paginator is self-contained, then you can use it as a library, move it to its own module and create as many paginators as you need without worrying about corrupt state.
Tightly-scoped data and loose coupling makes it easy to localize bugs.
Separate responsibility
With pagination, it's typically the backend database that should pick out the subset of records. Typically, your API will have routes for getting a total count of records, then offset and limit parameters, something like /api/posts?offset=30&limit=10
.
However, this paginator seems to expect to work with a whole data set, only to pick out and render a page with let dataSlice = data.list.slice(sliceStart, sliceEnd);
.
You could pull out the list-rendering logic from the paginator and let the caller deal with it as they see fit. The paginator only needs to fire a page change event. No need to pass the data in, slice it or assume anything else about it.
Use caution with template strings, innerHTML
and +=
I'm a huge fan of template strings with innerHTML
. It offers an OK chunk of the syntactic power and cleanliness of JSX in vanilla JS.
These tools aren't entirely free lunch, though. One problem is potential performance issues. Setting .innerHTML
means the string is parsed and converted to DOM elements under the hood. +=
can be potentially slow because of Shlemiel the painter's algorithm. We don't have a React diffing algorithm to selectively rerender only specific children.
These aren't immediate issues and engine optimizations make +=
and .innerHTML
as fast as possible, but keep this in mind when profiling if you notice performance problems later.
Unlike JSX, template strings and .innerHTML
don't sanitize anything, so be sure to sanitize to prevent XSS attacks if the data isn't trustworthy.
Short of React/Vue/etc, you could try a JSX-like library like htm to solve some of these problems.
Check out existing packages
When reinventing the wheel, it's a good idea to skim libraries on npm and GitHub and see what the state of the art is. This can steer you clear of poor designs and towards good ones up front. There's value in doing it the hard way blindly, but check out other designs afterwards.
For example, react-paginate accepts a click handler for page navigation and doesn't actually render the list. Even if you don't code React, you can probably see how this works on a high level from their demo.
Rewrite suggestion
This isn't an industrial-strength paginator and could benefit from validating parameters, but it seems good enough to illustrate my points.
const paginate = ({
container,
totalPages,
pagesToDisplay,
startPage,
onPageChange,
classes,
prevText,
nextText,
}) => {
const list = (size, cb) => Array(size).fill().map(cb).join("");
const makePaginationHTML = (currentPage, start) => `
${currentPage === 0 ? "" : `
<a href="#" class="${classes.prev}">${prevText}</a>
`}
${list(pagesToDisplay, (_, i) => `
<a href="#" class="${classes.item}">${start + i + 1}</a>
`)}
${currentPage === totalPages - 1 ? "" : `
<a href="#" class="${classes.next}">${nextText}</a>
`}
`;
const addPaginationListeners = (els, currentPage, start) => {
[...els].forEach((e, i) => {
start + i === currentPage && e.classList.add("active");
e.addEventListener("click", () => changePage(start + i));
});
container.querySelector("." + classes.prev)
?.addEventListener("click", () => changePage(currentPage - 1))
;
container.querySelector("." + classes.next)
?.addEventListener("click", () => changePage(currentPage + 1))
;
};
const renderPagination = currentPage => {
const half = Math.floor(pagesToDisplay / 2);
const start = currentPage >= totalPages - half
? totalPages - pagesToDisplay
: Math.max(0, currentPage - half)
;
container.innerHTML = makePaginationHTML(currentPage, start);
addPaginationListeners(
container.getElementsByClassName(classes.item),
currentPage,
start
);
};
const changePage = nextPage => {
renderPagination(nextPage);
onPageChange(nextPage);
};
changePage(startPage);
};
const renderPages = (currentPage, totalPages) => {
document.querySelector(".pagination-tip").innerHTML = `
<strong class="emp">${currentPage + 1}</strong> / ${totalPages}
`;
};
const renderList = data => {
document.querySelector(".list").innerHTML = data.map(e => `
<li>
<h5 class="title">
<small>${e.id} - </small>${e.title}
</h5>
</li>
`).join("");
};
(() => {
const data = {
list: [
{id: 1, title: 2001},
{id: 2, title: 2002},
{id: 3, title: 2003},
{id: 4, title: 2004},
{id: 5, title: 2005},
{id: 6, title: 2006},
{id: 7, title: 2007},
{id: 8, title: 2008},
{id: 9, title: 2009},
{id: 10, title: 2010},
{id: 11, title: 2011},
{id: 12, title: 2012},
{id: 13, title: 2013},
{id: 14, title: 2014},
{id: 15, title: 2015},
{id: 16, title: 2016},
{id: 17, title: 2017},
{id: 18, title: 2018},
{id: 19, title: 2019},
{id: 20, title: 2020},
{id: 21, title: 2021},
]
};
const limit = 4;
const totalPages = Math.ceil(data.list.length / limit);
const onPageChange = currentPage => {
// fetch data from server here
const offset = limit * currentPage;
renderList(data.list.slice(offset, offset + limit));
renderPages(currentPage, totalPages);
};
paginate({
container: document.querySelector(".pagination"),
totalPages,
pagesToDisplay: 5,
startPage: 0,
onPageChange,
classes: {
prev: "prev",
next: "next",
item: "item-link",
},
prevText: "←",
nextText: "→",
});
})();
* {
margin: 0;
padding: 0;
border: none;
font-size: 12px;
list-style: none;
text-decoration: none;
}
body {
padding: 20px;
}
.title {
font-size: 36px;
color: #f80;
}
.title > small {
font-size: 24px;
color: #ccc;
vertical-align: 4px;
}
.tip {
color: #999;
}
.emp {
color: #f80;
}
.list > li {
padding: 6px;
border-bottom: 1px solid #eee;
}
.pagination {
display: inline-block;
padding: 4px;
}
.pagination a {
display: inline-block;
padding: 4px 8px;
color: #00c;
}
.pagination a.active {
background: red;
font-weight: bold;
color: #f80;
background-color: #eee;
}
.pagination-tip {
margin: 0 0 0 10px;
}
<ul class="list"></ul>
<section class="pagination"></section>
<span class="pagination-tip"></span>