completed the task #1477
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,45 @@ | ||
| import React, { useState } from 'react'; | ||
| import './App.css'; | ||
| import { Pagination } from './components/Pagination'; | ||
| import { getNumbers } from './utils'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items: string[] = getNumbers(0, 42).map(n => `Item ${n}`); | ||
|
There was a problem hiding this comment. The items array is built with |
||
|
|
||
|
Comment on lines
4
to
8
There was a problem hiding this comment. The per-page |
||
| export const App: React.FC = () => { | ||
| const [perPage, changePerPage] = useState(5); | ||
| const [currentPage, changeCurrentPage] = useState(1); | ||
|
Comment on lines
+10
to
+11
There was a problem hiding this comment. Pagination state must be persisted in the URL using React Router query params. Initialize page and perPage from the URL on load (useSearchParams or useLocation) and update the query string whenever they change. Currently state is local only, so add router-based syncing here when initializing the states. |
||
| const startIndex = (currentPage - 1) * perPage + 1; | ||
|
There was a problem hiding this comment. |
||
| const nowItems = items.slice(startIndex, startIndex + perPage); | ||
|
There was a problem hiding this comment. nowItems slice uses the wrong startIndex (because startIndex above is 1-based). After fixing startIndex to be zero-based, slice like |
||
| const total = 42; | ||
| let ends = perPage * currentPage; | ||
|
|
||
|
Comment on lines
+11
to
+16
There was a problem hiding this comment. Pagination currently doesn't render the per-page selector at all. Per the Implementation Details, render the inside Pagination and control it via props (perPage, onPerPageChange). Add those props to the component type and render the options 3, 5, 10, 20. |
||
| if (ends > total) { | ||
|
Comment on lines
+11
to
+17
There was a problem hiding this comment. The component currently requires |
||
| ends = total; | ||
| } | ||
|
|
||
| return ( | ||
| <div className="container"> | ||
| <h1>Items with Pagination</h1> | ||
|
|
||
| <p className="lead" data-cy="info"> | ||
| Page {currentPage} (items {startIndex} - {ends} of {total}) | ||
|
There was a problem hiding this comment. The info text displays |
||
| </p> | ||
|
There was a problem hiding this comment. Similarly, the |
||
|
|
||
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| <select | ||
| value={perPage} | ||
| data-cy="perPageSelector" | ||
| id="perPageSelector" | ||
| className="form-control" | ||
| onChange={e => { | ||
| changeCurrentPage(1); | ||
| changePerPage(+e.target.value); | ||
| }} | ||
|
There was a problem hiding this comment. The numbered page |
||
| > | ||
| <option value="3">3</option> | ||
| <option value="5"> 5 </option> | ||
| <option value="10">10</option> | ||
| <option value="20">20</option> | ||
|
Comment on lines
+39
to
44
There was a problem hiding this comment. The click handler for selecting a page is attached to the |
||
| </select> | ||
|
Comment on lines
31
to
45
There was a problem hiding this comment. The per-page is rendered here in App, but the task requires the per-page selector to be rendered inside the stateless Pagination component and controlled via props (e.g. pass perPage and onPerPageChange). Move this selector into Pagination and remove it from App. |
||
|
|
@@ -32,78 +50,23 @@ export const App: React.FC = () => { | |
| </label> | ||
|
There was a problem hiding this comment. This anchor uses href={ |
||
| </div> | ||
|
|
||
| <Pagination | ||
| total={total} | ||
| perPage={perPage} | ||
| currentPage={currentPage} | ||
| onPageChange={page => { | ||
| changeCurrentPage(page); | ||
| }} | ||
| /> | ||
|
Comment on lines
+53
to
+60
There was a problem hiding this comment. When using the Pagination component you should also pass an onPerPageChange prop (after you move the selector into Pagination). Right now Pagination receives perPage/currentPage/onPageChange only; add and pass onPerPageChange from App so App can update perPage (and reset to page 1). |
||
|
|
||
| <ul> | ||
| {nowItems.map((item, index) => { | ||
| return ( | ||
| <li data-cy="item" key={index}> | ||
|
There was a problem hiding this comment. The There was a problem hiding this comment. Minor: when rendering the items list in App, prefer using a stable key such as the |
||
| {item} | ||
| </li> | ||
| ); | ||
| })} | ||
| </ul> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,76 @@ | ||
| import React from 'react'; | ||
|
There was a problem hiding this comment. The app currently has no React Router usage. The task requires saving |
||
| import { getNumbers } from '../../utils'; | ||
|
|
||
| type Props = { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage?: number; | ||
| onPageChange: (page: number) => void; | ||
|
There was a problem hiding this comment. Pagination props are missing an onPerPageChange (or similar) to allow App to control per-page value from the selector if you move it into Pagination. Add an |
||
| }; | ||
|
Comment on lines
+4
to
+9
There was a problem hiding this comment. The |
||
|
|
||
| export const Pagination: React.FC<Props> = ({ | ||
|
Comment on lines
+9
to
+11
There was a problem hiding this comment. React Router query param syncing is missing. The App should read |
||
| total, | ||
|
There was a problem hiding this comment. The |
||
| perPage, | ||
|
Comment on lines
+12
to
+13
There was a problem hiding this comment. The There was a problem hiding this comment. The slice call is off by one. You compute a zero-based startIndex (correct) but then use startIndex + 1 when slicing items. Use items.slice(startIndex, startIndex + perPage) so the displayed items match the info text and tests. |
||
| currentPage = 1, | ||
| onPageChange, | ||
| }) => { | ||
|
Comment on lines
+11
to
+16
There was a problem hiding this comment. Provide a default value for |
||
| const buttons = getNumbers(1, Math.ceil(total / perPage)); | ||
|
|
||
| return ( | ||
| <ul className="pagination"> | ||
| <li className={currentPage === 1 ? 'page-item disabled' : 'page-item'}> | ||
|
There was a problem hiding this comment. It's better to use |
||
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1 ? 'true' : 'false'} | ||
|
There was a problem hiding this comment. The info paragraph uses |
||
| onClick={() => { | ||
| if (currentPage !== 1) { | ||
| onPageChange(currentPage - 1); | ||
| } | ||
| }} | ||
| > | ||
| « | ||
| </a> | ||
| </li> | ||
| {buttons.map(button => { | ||
| return ( | ||
|
Comment on lines
+36
to
+38
There was a problem hiding this comment. When perPage changes you reset the current page to 1 (good), but you also need to update the URL query string (page=1&perPage=...) so the new state is persisted. Integrate this into the onChange handler (or centralize URL updates in App's state effects). |
||
| <li | ||
|
Comment on lines
+31
to
+39
There was a problem hiding this comment. The per-page selector must be rendered inside the stateless Pagination component and controlled via props (e.g. pass an onPerPageChange handler). Currently the lives in App; move it into Pagination and drive it via props so Pagination owns the UI for per-page selection as required. |
||
| onClick={() => { | ||
| if (button !== currentPage) { | ||
| onPageChange(button); | ||
| } | ||
| }} | ||
| className={ | ||
| currentPage === button ? 'page-item active' : 'page-item' | ||
|
There was a problem hiding this comment. It's better to use |
||
| } | ||
| key={button} | ||
| > | ||
| <a data-cy="pageLink" className="page-link" href={`#${button}`}> | ||
|
There was a problem hiding this comment. Numbered page anchors ( href that updates the hash. This violates the requirement to prevent default navigation. Move the click handler to the anchor (or call event.preventDefault() inside it) so clicks do not change the URL hash. |
||
| {button} | ||
| </a> | ||
|
Comment on lines
+37
to
+52
There was a problem hiding this comment. In the Pagination component the numbered anchors use |
||
| </li> | ||
| ); | ||
| })} | ||
| <li | ||
| className={ | ||
| currentPage === buttons.length ? 'page-item disabled' : 'page-item' | ||
|
There was a problem hiding this comment. It's better to use |
||
| } | ||
| > | ||
| <a | ||
| data-cy="nextLink" | ||
| className="page-link" | ||
| href="#next" | ||
| aria-disabled={currentPage === buttons.length ? 'true' : 'false'} | ||
| onClick={() => { | ||
|
There was a problem hiding this comment. You use |
||
| if (currentPage !== buttons.length) { | ||
|
Comment on lines
+64
to
+68
There was a problem hiding this comment. Using the array |
||
| onPageChange(currentPage + 1); | ||
| } | ||
| }} | ||
| > | ||
| » | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| ); | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The app currently doesn't persist or apply
?page=...&perPage=...in the URL. The task explicitly requires using React Router to save query params on change and apply them on load. You need to read initial values from the location (query params) and push/update the URL when page or perPage changes (e.g. usinguseSearchParams/useLocation+useNavigateoruseHistory).