-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Solution1 #1482
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?
Solution1 #1482
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,57 @@ | ||
| import React from 'react'; | ||
| import './App.css'; | ||
| import { getNumbers } from './utils'; | ||
| import { Pagination } from './components/Pagination'; | ||
| import { useSearchParams } from 'react-router-dom'; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const items = getNumbers(1, 42).map(n => `Item ${n}`); | ||
|
|
||
|
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. Typo in function name. If keeping a separate handler, rename to |
||
| export const App: React.FC = () => { | ||
| const [searchParams, setSearchParams] = useSearchParams({ | ||
|
Comment on lines
+4
to
+11
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. Props type includes unnecessary fields. Required API: |
||
| perPage: '5', | ||
| page: '1', | ||
| }); | ||
|
|
||
| const page = searchParams.get('page') || '1'; | ||
| const perPage = searchParams.get('perPage') || '5'; | ||
| const currentPage = Number(page); | ||
|
|
||
| const startIndex = (currentPage - 1) * Number(perPage); | ||
| const endIndex = Math.min(Number(perPage) + startIndex, items.length); | ||
|
|
||
| const updateParams = (key: string, value: string) => { | ||
| const params = new URLSearchParams(searchParams); | ||
|
|
||
| params.set(key, value); | ||
| setSearchParams(params); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="container"> | ||
| <h1>Items with Pagination</h1> | ||
|
Comment on lines
31
to
32
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. In Pagination mapping, each page value={page} (line 47) and the component prop page is a string. The value attribute on li isn't required and it's incorrect to assign the whole current page string to each list item. Remove value={page} or replace with a meaningful attribute (not required). This is unnecessary and may confuse logic. |
||
|
|
||
| <p className="lead" data-cy="info"> | ||
| Page 1 (items 1 - 5 of 42) | ||
| Page {page} (items {startIndex + 1} - {endIndex} of {items.length}) | ||
|
Comment on lines
34
to
+35
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. In App, |
||
| </p> | ||
|
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 |
||
|
|
||
| <div className="form-group row"> | ||
| <div className="col-3 col-sm-2 col-xl-1"> | ||
| {/* важливо! оновленя двох параметрів */} | ||
| <select | ||
| data-cy="perPageSelector" | ||
| id="perPageSelector" | ||
|
Comment on lines
38
to
43
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 currently updates the URL via updateParams inside page links (updateParams('page', String(pageNumber)) onClick). Per task, the Pagination should be stateless and only call onPageChange. The App should listen and save new page (possibly into URL). Move URL updates to App (i.e., call onPageChange and let App call updateParams). |
||
| className="form-control"> | ||
| className="form-control" | ||
| value={perPage} | ||
| onChange={e => { | ||
| const params = new URLSearchParams(); | ||
|
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. Remove the |
||
|
|
||
| params.set('perPage', e.target.value); | ||
|
Comment on lines
+47
to
+49
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. Use |
||
| params.set('page', '1'); | ||
|
|
||
| setSearchParams(params); | ||
| }} | ||
|
Comment on lines
+46
to
+53
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. move to separate fn |
||
| > | ||
| <option value="3">3</option> | ||
| <option value="5">5</option> | ||
| <option value="10">10</option> | ||
|
|
@@ -32,78 +64,23 @@ export const App: React.FC = () => { | |
| </label> | ||
| </div> | ||
|
|
||
| {/* 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> | ||
| <Pagination | ||
| total={items.length} | ||
| perPage={Number(perPage)} | ||
| onPageChange={newPage => { | ||
| updateParams('page', String(newPage)); | ||
| }} | ||
| currentPage={currentPage} | ||
|
Comment on lines
+67
to
+73
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 should only receive the 4 required props: |
||
| /> | ||
|
Comment on lines
+67
to
+74
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. App's Pagination usage (lines 83-90) passes |
||
|
|
||
| <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> | ||
| {items.slice(startIndex, endIndex).map(item => { | ||
| return ( | ||
| <li key={item} data-cy="item"> | ||
| {item} | ||
| </li> | ||
| ); | ||
| })} | ||
| </ul> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,91 @@ | ||
| export const Pagination = () => {}; | ||
| import classNames from 'classnames'; | ||
| import React from 'react'; | ||
|
|
||
| type Props = { | ||
| total: number; | ||
| perPage: number; | ||
| onPageChange: (page: number) => void; | ||
| currentPage: number; | ||
|
Comment on lines
+4
to
+8
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. Props type still contains incorrect props not in the required API: |
||
| }; | ||
|
|
||
| export const Pagination: React.FC<Props> = ({ | ||
|
Comment on lines
+4
to
+11
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 Pagination component's props and behavior don't match the task description. The description requires a stateless Pagination that accepts props: total, perPage, currentPage (optional default 1), and onPageChange (callback). In this file the component expects: |
||
| total, | ||
| perPage, | ||
| onPageChange, | ||
| currentPage, | ||
|
Comment on lines
+11
to
+15
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. Destructuring still includes non-standard props. Only destructure |
||
| }) => { | ||
|
Comment on lines
+11
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. The component currently receives |
||
| const totalPage = Math.ceil(total / perPage); | ||
| const pages = Array.from({ length: totalPage }, (__dirname, i) => i + 1); | ||
| const lastPage = currentPage === totalPage; | ||
|
|
||
| return ( | ||
| <ul className="pagination"> | ||
| <li | ||
| className={classNames('page-item', { | ||
| disabled: currentPage === 1, | ||
| })} | ||
| > | ||
| <a | ||
| data-cy="prevLink" | ||
| className="page-link" | ||
| href="#prev" | ||
| aria-disabled={currentPage === 1 ? 'true' : 'false'} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| if (currentPage > 1) { | ||
| onPageChange(currentPage - 1); | ||
| } | ||
| }} | ||
|
Comment on lines
+37
to
+43
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. move to separate fn |
||
| > | ||
| « | ||
| </a> | ||
|
Comment on lines
+32
to
+46
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 |
||
| </li> | ||
|
|
||
| {pages.map(pageNumber => { | ||
| return ( | ||
| <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. Remove this
Comment on lines
+50
to
+51
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. Missing 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. Remove |
||
| className={classNames('page-item', { | ||
| active: currentPage === pageNumber, | ||
| })} | ||
| key={pageNumber} | ||
|
Comment on lines
+51
to
+55
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.
|
||
| > | ||
| <a | ||
| data-cy="pageLink" | ||
| className="page-link" | ||
| href={`#${pageNumber}`} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
| onPageChange(pageNumber); | ||
|
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. Use only 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. move to separate fn |
||
| }} | ||
|
Comment on lines
+57
to
+65
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 uses |
||
| > | ||
| {pageNumber} | ||
| </a> | ||
| </li> | ||
| ); | ||
| })} | ||
| <li | ||
| className={classNames('page-item', { | ||
| disabled: lastPage, | ||
| })} | ||
| > | ||
| <a | ||
| data-cy="nextLink" | ||
| className="page-link" | ||
| href="#next" | ||
| aria-disabled={lastPage ? 'true' : 'false'} | ||
| onClick={e => { | ||
| e.preventDefault(); | ||
|
|
||
| if (!lastPage) { | ||
|
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. Fix the callback parameter name typo |
||
| onPageChange(currentPage + 1); | ||
| } | ||
| }} | ||
|
Comment on lines
+82
to
+88
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. move to separate fn |
||
| > | ||
| » | ||
| </a> | ||
|
Comment on lines
+82
to
+91
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. Pass only the required props: |
||
| </li> | ||
| </ul> | ||
| ); | ||
| }; | ||
|
|
||
| export default Pagination; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,11 @@ | ||
| import { createRoot } from 'react-dom/client'; | ||
| import React from 'react'; | ||
|
|
||
| import { App } from './App'; | ||
|
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 uses prop names and prop types for Pagination that do not match the task description. The task required a stateless API. Here the Pagination is used with props |
||
| import { HashRouter } from 'react-router-dom'; | ||
|
|
||
|
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. Remove this prop - it's not part of the required API. The component should use |
||
| createRoot(document.getElementById('root') as HTMLElement).render(<App />); | ||
| createRoot(document.getElementById('root') as HTMLElement).render( | ||
| <HashRouter> | ||
|
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. Remove this prop - it's not part of the required API. Use |
||
| <App /> | ||
|
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. Remove this prop - it's not part of the required API. Compute disabled state internally as |
||
| </HashRouter>, | ||
| ); | ||
|
Comment on lines
4
to
+11
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. Props type includes wrong props. Task requires only: |
||
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 component uses search params to manage page and perPage, but the task requires a stateless Pagination component that accepts props: total, perPage, currentPage (optional), and onPageChange. Here Pagination's props are different (pages, page, updateParams, handlePgeChange, lastPage, currentPage). This does not match the required API from the description — update the Pagination to accept the specified props (total, perPage, currentPage, onPageChange) or adjust the App to pass exactly those props as described in the task. (See checklist Core Functional Requirements.)