-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Solution #1479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Solution #1479
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,110 +1,53 @@ | ||
| import React from 'react'; | ||
| import React, { useState } from 'react'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The component currently only imports React. To meet the requirement that |
||
| import './App.css'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the props interface name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename the props interface |
||
| import { getNumbers } from './utils'; | ||
| import { Pagination } from './components/Pagination'; | ||
| import { ItemList } from './components/ItemList/ItemList'; | ||
| import { PageInfo } from './components/PageInfo'; | ||
| import { PerPageSelector } from './components/PerPageSelector/PerPageSelector'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
|
|
||
| export const App: React.FC = () => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defaulting |
||
| const [currentPage, setCurrentPage] = useState(1); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The app initializes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not initialize |
||
| const [perPage, setPerPage] = useState(5); | ||
|
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't initialize There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not initialize |
||
| const startIndex = (currentPage - 1) * perPage; | ||
| const lastIndex = Math.min(startIndex + perPage, items.length); | ||
|
Comment on lines
+15
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Computing |
||
| const visibleItems = items.slice(startIndex, lastIndex); | ||
|
|
||
| const handlePageChange = (page: number) => { | ||
| setCurrentPage(page); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| }; | ||
|
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| const handlePerPageChange = (pages: number) => { | ||
| setPerPage(pages); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| setCurrentPage(1); | ||
| }; | ||
|
Comment on lines
+23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| return ( | ||
| <div className="container"> | ||
| <h1>Items with Pagination</h1> | ||
|
|
||
| <p className="lead" data-cy="info"> | ||
| Page 1 (items 1 - 5 of 42) | ||
| </p> | ||
|
|
||
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| <select | ||
| data-cy="perPageSelector" | ||
| id="perPageSelector" | ||
| className="form-control"> | ||
| <option value="3">3</option> | ||
| <option value="5">5</option> | ||
| <option value="10">10</option> | ||
| <option value="20">20</option> | ||
| </select> | ||
| </div> | ||
| <PageInfo | ||
| currentPage={currentPage} | ||
| startIndex={startIndex} | ||
| lastIndex={lastIndex} | ||
| total={items.length} | ||
| /> | ||
|
|
||
| <label htmlFor="perPageSelector" className="col-form-label col"> | ||
| items per page | ||
| </label> | ||
| </div> | ||
| <PerPageSelector | ||
| perPage={perPage} | ||
| onPerPageChange={handlePerPageChange} | ||
| /> | ||
|
|
||
| {/* Move this markup to Pagination */} | ||
| <ul className="pagination"> | ||
| <li className="page-item disabled"> | ||
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled="true"> | ||
| « | ||
| </a> | ||
| </li> | ||
| <li className="page-item active"> | ||
| <a data-cy="pageLink" className="page-link" href="#1"> | ||
| 1 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#2"> | ||
| 2 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#3"> | ||
| 3 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#4"> | ||
| 4 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#5"> | ||
| 5 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#6"> | ||
| 6 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#7"> | ||
| 7 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#8"> | ||
| 8 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a data-cy="pageLink" className="page-link" href="#9"> | ||
| 9 | ||
| </a> | ||
| </li> | ||
| <li className="page-item"> | ||
| <a | ||
| data-cy="nextLink" | ||
| className="page-link" | ||
| href="#next" | ||
| aria-disabled="false"> | ||
| » | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| <ul> | ||
| <li data-cy="item">Item 1</li> | ||
| <li data-cy="item">Item 2</li> | ||
| <li data-cy="item">Item 3</li> | ||
| <li data-cy="item">Item 4</li> | ||
| <li data-cy="item">Item 5</li> | ||
| </ul> | ||
| <Pagination | ||
| total={items.length} | ||
| perPage={perPage} | ||
| currentPage={currentPage} | ||
| onPageChange={handlePageChange} | ||
|
Comment on lines
+44
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You pass |
||
| /> | ||
| <ItemList visibleItems={visibleItems} /> | ||
| </div> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The page link click handler calls |
||
| ); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import React from 'react'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The task requires using React Router to save and apply query params (e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The task requires reading and writing pagination state from/to the URL using React Router (useSearchParams or useLocation/useNavigate). This file currently has no react-router usage: import and use the appropriate hooks so you can parse ?page and ?perPage on load and update the query string when the user changes page or perPage (check checklist items #13, #14, #15, #16, #17, #26). |
||
| interface VisiblevisibleItems { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interface name |
||
| visibleItems: string[]; | ||
| } | ||
| export const ItemList: React.FC<VisiblevisibleItems> = ({ visibleItems }) => { | ||
| return ( | ||
| <ul> | ||
| {visibleItems.map(item => ( | ||
| <li data-cy="item" key={item}> | ||
| {item} | ||
| </li> | ||
| ))} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The top-level component should read |
||
| </ul> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You initialize |
||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currentPage and perPage are initialized here with static defaults. Per checklist items #13/#25 you must instead initialize them from the URL query string on initial render, validating values (clamp page to [1, totalPages] and ensure perPage is one of 3,5,10,20). Use parsed & validated values to set initial state instead of always starting with 1 and 5. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You initialize |
||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| import React from 'react'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The task requires using React Router to save There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The App component currently doesn't import or use any React Router hooks. The task requires reading |
||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Optional: the interface name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the props interface name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the props interface name |
||
| interface PageInfoProps { | ||
| currentPage: number; | ||
| startIndex: number; | ||
| lastIndex: number; | ||
| total: number; | ||
| } | ||
| export const PageInfo: React.FC<PageInfoProps> = ({ | ||
| currentPage, | ||
| startIndex, | ||
| lastIndex, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pagination is implemented as a stateless component, which is good. However, ensure the top-level App is the single source of truth for |
||
| total, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not hardcode the initial |
||
| }) => { | ||
|
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. currentPage and perPage are initialized with fixed defaults here. Per requirements you must initialize these values from the URL query string on first render (parse There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not hardcode the initial |
||
| return ( | ||
| <p className="lead" data-cy="info"> | ||
| Page {currentPage} (items {startIndex + 1} - {lastIndex} of {total}) | ||
| </p> | ||
| ); | ||
| }; | ||
|
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the page changes the App updates internal state but does not update the URL. According to the requirements the App should save |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,76 @@ | ||
| export const Pagination = () => {}; | ||
| import React from 'react'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The task requires using React Router to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to read initial pagination state from the URL and persist it back when it changes. Import and use React Router hooks (e.g., |
||
| import cn from 'classnames'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor readability suggestion: rename the |
||
| interface PaginationProps { | ||
| total: number; | ||
| perPage: number; | ||
| currentPage?: number; | ||
| onPageChange: (page: number) => void; | ||
| } | ||
| export const Pagination: React.FC<PaginationProps> = ({ | ||
| total, | ||
| perPage, | ||
| currentPage = 1, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pagination is currently implemented as a stateless component which is good, but relying on its internal default |
||
| onPageChange, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| }) => { | ||
|
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The component/app does not read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't hardcode
Comment on lines
+13
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid initializing |
||
| const totalPages = Math.ceil(total / perPage); | ||
| const pages = []; | ||
|
|
||
|
Comment on lines
+15
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. startIndex/lastIndex/visibleItems are computed from the current state — make sure they are computed after you parse and validate |
||
| for (let i = 1; i <= totalPages; i++) { | ||
| pages.push(i); | ||
| } | ||
|
Comment on lines
+19
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard against no-op page changes and synchronize the URL. |
||
|
|
||
|
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When the page changes (
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The App-level handler should avoid unnecessary updates: add a guard |
||
| return ( | ||
| <ul className="pagination"> | ||
| <li className={cn('page-item', { disabled: currentPage === 1 })}> | ||
| <a | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate the incoming |
||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| if (currentPage > 1) { | ||
| onPageChange(currentPage - 1); | ||
| } | ||
| }} | ||
| > | ||
| « | ||
| </a> | ||
| </li> | ||
| {pages.map(page => ( | ||
| <li | ||
| className={cn('page-item', { active: page === currentPage })} | ||
| key={page} | ||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href={`#${page}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(page); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The page link onClick handler always calls |
||
| }} | ||
| > | ||
| {page} | ||
| </a> | ||
| </li> | ||
| ))} | ||
| <li className={cn('page-item', { disabled: currentPage === totalPages })}> | ||
| <a | ||
| data-cy="nextLink" | ||
| className="page-link" | ||
| href="#next" | ||
| aria-disabled={currentPage === totalPages} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| if (currentPage < totalPages) { | ||
| onPageChange(currentPage + 1); | ||
| } | ||
| }} | ||
| > | ||
| » | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import React from 'react'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The task requires using React Router to save and apply query params (e.g. |
||
| interface PerPageSelectorProps { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: the interface name |
||
| perPage: number; | ||
| onPerPageChange: (perPage: number) => void; | ||
| } | ||
| export const PerPageSelector: React.FC<PerPageSelectorProps> = ({ | ||
| perPage, | ||
| onPerPageChange, | ||
| }) => { | ||
| return ( | ||
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| <select | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| data-cy="perPageSelector" | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| id="perPageSelector" | ||
| className="form-control" | ||
| value={perPage} | ||
| onChange={e => onPerPageChange(+e.target.value)} | ||
|
Comment on lines
+17
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The select and options are implemented correctly. Ensure the parent (App) validates that the |
||
| > | ||
| <option value="3">3</option> | ||
| <option value="5">5</option> | ||
|
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| <option value="10">10</option> | ||
| <option value="20">20</option> | ||
| </select> | ||
| </div> | ||
|
Comment on lines
+23
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| <label htmlFor="perPageSelector" className="col-form-label col"> | ||
| items per page | ||
| </label> | ||
| </div> | ||
| ); | ||
| }; | ||
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.
You aren't using React Router here. To satisfy the starred requirement (#13/#25/#26) import and use React Router hooks (e.g.
useSearchParamsoruseLocation+useNavigate) so you can read?page=...&perPage=...on load and later update the URL when state changes.