2
\$\begingroup\$

I've recently started journey with ReactJS and to begin with I wanted to try something a bit more complex - paginated list of elements with back functionality enabled. Below are two main components that I'd like to get feedback about. Especially PagedListings.

import './App.css';
import PagedListings from './components/PagedListings';
import ScrollToTop from './components/ScrollToTop';
import React from "react";
import { Route } from 'react-router';
import { BrowserRouter as Router } from 'react-router-dom';

function App() {
  return (
    <Router>
    <ScrollToTop>
       <Route path={["/", "/:page"]} component={PagedListings}/>
    </ScrollToTop>
    </Router>
  );
}

export default App;

And the actual component executing planned task

import React, { useMemo } from "react";
import ListingsContainer from "./ListingsContainer";
import Pagination from "@material-ui/lab/Pagination";
import { useHistory, useLocation } from "react-router-dom";

const LISTINGS = ["Rec1", "Rec2", "Rec3",
  "Rec4", "Rec5", "Rec6", "Rec7", "Rec8",
  "Rec9", "Rec10",];
const DEFAULT_PAGE = 1;
const MAX_PAGE_COUNT = 10;

function PagedListings(props) {
  const history = useHistory();
  const location = useLocation();
  const urlParams = useMemo(() => {
    return new URLSearchParams(location.search);
  }, [location.search]);

  const handleChange = (
    event: React.MouseEvent<HTMLButtonElement> | null,
    newPage: number
  ) => {
    let oneBasedNewPage = newPage;

    urlParams.set("page", oneBasedNewPage.toString());
    updateURL();
  };

  const updateURL = () => {
    history.push({
      pathname: location.pathname,
      search: `?${urlParams}`,
    });
  };

  function getPageNumber() {
    let newPage = parseInt(urlParams.get("page")) || DEFAULT_PAGE;
    return newPage <= MAX_PAGE_COUNT ? newPage : MAX_PAGE_COUNT;
  }

  return (
    <>
      <ListingsContainer listings={LISTINGS.slice(0, getPageNumber())} />
      <Pagination
        count={MAX_PAGE_COUNT}
        defaultPage={DEFAULT_PAGE}
        page={getPageNumber()}
        onChange={handleChange}
      />
    </>
  );
}

export default PagedListings;
\$\endgroup\$
1
\$\begingroup\$

Your code looks quite reasonable. There are only a few small things I notice:

  • Indent the JSX in App a bit more: ScrollToTop is a child of Router, so it should be indented further, not on the same level as Router. (It doesn't really make a difference here, but it's good to be consistent, and could be an improvement if that section ever got more complicated)

  • Avoid let, prefer const - ESLint rule here.

  • Or, in the case of oneBasedNewPage, consider defining the argument with that name instead of creating a new variable - and maybe call it oneIndexed, not oneBased:

    const handleChange = (
      event: React.MouseEvent<HTMLButtonElement> | null,
      oneIndexedNewPage: number
    ) => {
    
  • URLSearchParams does not differentiate between numbers and strings.

    urlParams.set("page", oneBasedNewPage.toString());
    

    can be

    urlParams.set("page", oneIndexedNewPage);
    

    if you wish.

  • Consider if using Math.min might be clearer than the conditional operator:

    function getPageNumber() {
      const newPage = parseInt(urlParams.get("page")) || DEFAULT_PAGE;
      return Math.min(newPage, MAX_PAGE_COUNT);
    }
    
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your review and all of the suggestions. I haven't been aware of the fact to use const instead of let in place you suggested. \$\endgroup\$ – sebap123 Dec 22 '20 at 9:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.