Skip to content
This repository has been archived by the owner on Dec 7, 2020. It is now read-only.

feat: Add Pagination component #219

Merged
merged 1 commit into from
Nov 4, 2019
Merged

feat: Add Pagination component #219

merged 1 commit into from
Nov 4, 2019

Conversation

rafenden
Copy link
Contributor

This component was extracted from the collection list and will replace it in a follow-up PR.

image

@codecov
Copy link

codecov bot commented Oct 31, 2019

Codecov Report

Merging #219 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #219      +/-   ##
=========================================
+ Coverage   99.16%   99.2%   +0.04%     
=========================================
  Files          79      82       +3     
  Lines        1076    1136      +60     
  Branches      219     238      +19     
=========================================
+ Hits         1067    1127      +60     
  Misses          9       9
Impacted Files Coverage Δ
src/pagination/Pagination.jsx 100% <100%> (ø)
src/pagination/constants.js 100% <100%> (ø)
src/pagination/computeVisiblePieces.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aa0af0...1d134d9. Read the comment docs.

src/pagination/Pagination.jsx Outdated Show resolved Hide resolved
src/pagination/Pagination.jsx Outdated Show resolved Hide resolved
src/pagination/usePaginator.js Outdated Show resolved Hide resolved
src/pagination/Pagination.jsx Outdated Show resolved Hide resolved
let wrapper

describe('when 7 pages are passed and the active page is 3', () => {
beforeAll(() => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for beforeAll as all the tests only differ by assertions. It's actually a single test with multiple assertions. The "should render the ..." messages should actually be custom assertion messages. Unfortunately, Jest doesn't support this standard feature out of the box, but the jest-expect-message package solves this problem.

Apart from the tests to appear more complex than they really are, mounting the same stateless component 5 times just to make 5 assertions can affect the duration of the whole test suite. Especially if we do it everywhere, which we do. Just in this PR there are 24 mount calls, when it only could be 7.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The component it's mounted only once as beforeAll is executed once for all the test cases.

src/pagination/__tests__/Pagination.test.jsx Show resolved Hide resolved
@rafenden rafenden requested a review from peterhudec November 1, 2019 12:02
Comment on lines +37 to +39
& + & {
margin-left: ${SPACING.SCALE_1};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add this to the adjacent component rather than using SASS selectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was changed following comment added by @peterhudec #218 (comment)

@rafenden rafenden merged commit 168de37 into master Nov 4, 2019
@rafenden rafenden deleted the pagination branch November 4, 2019 11:52
data-hub-bot pushed a commit that referenced this pull request Nov 4, 2019
@data-hub-bot
Copy link

🎉 This PR is included in version 2.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants