feat: implement pagination #321
Conversation
|
Code Climate has analyzed commit f9e9eab and detected 4 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
src/components/Pagination/index.js
Outdated
|
|
||
| return ( | ||
| <div className={getContainerClassNames()} style={style}> | ||
| <ButtonIcon |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
src/components/Pagination/index.js
Outdated
| onClick={() => onChange(activePage - 1)} | ||
| disabled={isFirstItemSelected} /> | ||
| <PageButtons onChange={onChange} pages={pages} activePage={activePage} /> | ||
| <ButtonIcon |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| ); | ||
| } | ||
|
|
||
| PageButtons.propTypes = { |
There was a problem hiding this comment.
Similar blocks of code found in 2 locations. Consider refactoring.
| @@ -0,0 +1,8 @@ | |||
| export default function getFirstItem(pages, activePage) { | |||
| if (activePage < 3) { | |||
| return activePage - (activePage - 1); | |||
There was a problem hiding this comment.
here you can return 1 since all posibilities are 1
|
|
||
| const renderButtons = () => { | ||
| const firstItem = getFirstItem(pages, activePage); | ||
| return new Array(5).fill(0).map((item, index) => ( |
There was a problem hiding this comment.
you don't need to use new, it's better to use Array(5) without new keyword
| key={`page-button-${firstItem + index}`} | ||
| label={firstItem + index} | ||
| className={getButtonClassName(firstItem + index)} | ||
| onClick={() => onChange(firstItem + index)} /> |
There was a problem hiding this comment.
onClick={(event) => onChange(event, firstItem + index)}
src/components/Pagination/index.js
Outdated
| variant="border-filled" | ||
| className="rainbow-pagination_button rainbow-pagination_button-icon" | ||
| icon={<LeftArrow />} | ||
| onClick={() => onChange(activePage - 1)} |
There was a problem hiding this comment.
here pass the event too and the same above
There was a problem hiding this comment.
I will say that the we should use a hidden input inside then we change the value on the input in order to trigger the input change event
|
|
||
| const getClassName = () => classnames( | ||
| 'rainbow-pagination_navigation-button', | ||
| { 'rainbow-pagination_navigation-button--disabled': disabled }, |
There was a problem hiding this comment.
const getClassName = () => classnames('rainbow-pagination_navigation-button', {
'rainbow-pagination_navigation-button--disabled': disabled,
});
| const getButtonClassName = (firstItem, index, buttonsToRender) => ( | ||
| classnames( | ||
| 'rainbow-pagination_button', | ||
| { |
There was a problem hiding this comment.
same ^^:
classnames('rainbow-pagination_button', {
'rainbow-pagination_button--first': index === 0,
'rainbow-pagination_button--last': index === buttonsToRender - 1,
'rainbow-pagination_button--active': activePage === firstItem + index,
};
and if this function does't depend on any prop then move outside the component above
| return Array(buttonsToRender).fill(0).map((item, index) => { | ||
| const page = firstItem + index; | ||
| return ( | ||
| <li key={`page-button-${page}`} className={getButtonClassName(firstItem, index, buttonsToRender)}> |
There was a problem hiding this comment.
put this in a const
const key = `page-button-${page}`;
| return Array(buttonsToRender).fill(0).map((item, index) => { | ||
| const page = firstItem + index; | ||
| return ( | ||
| <li key={`page-button-${page}`} className={getButtonClassName(firstItem, index, buttonsToRender)}> |
There was a problem hiding this comment.
since inside getButtonClassName you use firstItem + index then pass the constant page directly here
| <a | ||
| onClick={event => onChange(event, page)} | ||
| aria-current={getAriaCurrent(page)} | ||
| aria-label={`Goto Page ${page}`} |
There was a problem hiding this comment.
same ^^ put in a variable
| <GlobalHeader src="images/user/user3.jpg" /> | ||
| <div className="rainbow-p-around_x-large rainbow-p-around_x-large"> | ||
| <Pagination className="rainbow-m_auto" pages={7} activePage={activePage} onChange={this.handleOnChange} /> | ||
| </div> |
There was a problem hiding this comment.
We need to add the examples that are in the zeplin
| } | ||
| } | ||
|
|
||
| .rainbow-pagination_button--first { |
There was a problem hiding this comment.
why you need add this classNames. You can use a pseudo-clase for example
the first element
.rainbow-pagination_button:nth-of-type(1)
the last element
.rainbow-pagination_button:nth-last-of-type(1)
| width: 100%; | ||
|
|
||
| svg { | ||
| width: 7px !important; |
There was a problem hiding this comment.
why you use !important
|
|
||
| @extend .center-items; | ||
|
|
||
| a { |
There was a problem hiding this comment.
why you put styles to the a element, this is a bad practice instead add a className to this element
| height: 100%; | ||
| text-decoration: none; | ||
| font-weight: bold; | ||
| font-size: $font-size-text-large; |
There was a problem hiding this comment.
font-size: $font-size-text-large it is the style of the font when is active, in the normal state it is font-size: $font-size-text-medium
| className={getButtonClassName(page, activePage, index, buttonsToRender)}> | ||
| <a | ||
| onClick={event => onChange(event, page)} | ||
| aria-current={getAriaCurrent(page)} |
There was a problem hiding this comment.
here you forgot to pass activePage
fixes #316