Skip to content
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

feat(Pagination): add new component #2352

Merged
merged 6 commits into from
Jan 10, 2018
Merged

feat(Pagination): add new component #2352

merged 6 commits into from
Jan 10, 2018

Conversation

layershifter
Copy link
Member

This PR adds the fully featured pagination to SUIR using existing Menu component.

@layershifter
Copy link
Member Author

layershifter commented Dec 3, 2017

@levithomason I'll be glad to hear your opinion there 😄

@codecov-io
Copy link

codecov-io commented Dec 10, 2017

Codecov Report

Merging #2352 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2352      +/-   ##
==========================================
+ Coverage   99.73%   99.73%   +<.01%     
==========================================
  Files         152      154       +2     
  Lines        2664     2680      +16     
==========================================
+ Hits         2657     2673      +16     
  Misses          7        7
Impacted Files Coverage Δ
src/addons/Pagination/Pagination.js 100% <100%> (ø)
src/addons/Pagination/PaginationItem.js 100% <100%> (ø)
src/elements/Button/ButtonGroup.js 100% <0%> (ø) ⬆️

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 4f2f3fc...18dd654. Read the comment docs.

@levithomason
Copy link
Member

This is so cool and much needed! Here are my initial thoughts when trying the component.

showFirstAndLast

I would expect this prop to show the first and last page number in the list of pages. However, that is already shown. This prop seems to add first and last navigation buttons but those are duplicating the first and last page number buttons:

image

💡 Suggestion:

Perhaps split this into showFirstAndLastNav and showFirstAndLastPage?

showPreviousAndNext

Since the item prop is prevItem I think we should use showPrevAndNext. Also, if we split the first/last nav vs page props above, then this would be showPrevAndNextNav for consistency.

Item shorthand

I can't seem to turn off the first/last page number. Passing null/false still renders them. I think we should allow turning these off.

<Pagination firstItem={null} lastItem={false} />

image

Oh, right. You'd pass showPreviousAndNext={false} 😊. Do we even need the show* props? The *Item props could work as <Dropdown icon={null} /> and <Modal dimmer={false} /> which removes the shorthand items. Thoughts?

@layershifter
Copy link
Member Author

@levithomason I've applied proposed changes and removed all boolean show* props 👍

@levithomason levithomason merged commit 4c8fc4d into master Jan 10, 2018
@levithomason levithomason deleted the feat/pagination branch January 10, 2018 07:52
@levithomason
Copy link
Member

Cool, great first implementation!

@levithomason
Copy link
Member

Released in semantic-ui-react@0.77.2

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

Successfully merging this pull request may close these issues.

3 participants