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

Show table overflow only if limit exceeded by 2+ #2683

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

rndstr
Copy link
Contributor

@rndstr rndstr commented Jul 5, 2017

Having to toggle a +1 feels weird while that +1 takes up
the same vertical space as displaying that row.


Just something small I came across.

Behavior on default (or configured) row limit equals 5:

Items Display,Overflow (Alternative)
4 4,0 4,0
5 5,0 5,0
6 6,0 4,+2
7 5,+2 4,+3
8 5,+3 4,+4

Current implementation shows generally $limit rows, alternative would be if you want a max row (including toggle) to not exceed $limit.

@rade rade requested a review from foot July 6, 2017 08:50
Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

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

Cool idea :)

It still shows the +1 row atm. this.state.limit is still getting used around the place but has in a sense changed.

Having to toggle a +1 feels weird while that +1 takes up
the same vertical space as displaying that row.
@rndstr rndstr force-pushed the table-overflow-plus-one branch from 7a69f80 to dfda3e0 Compare July 6, 2017 11:56
@rndstr
Copy link
Contributor Author

rndstr commented Jul 6, 2017

@foot thanks! I missed that notShown should've been adjusted as well.

(I now also stumbled over a bug that if you pass a non-default props value for limit, after you toggle twice it would return back to default instead of the value you passed in initially.)

@foot
Copy link
Contributor

foot commented Jul 6, 2017

Yeah good catch w/ the default prop.

Looks good now and works really nicely! LGTM

@rndstr rndstr merged commit cf8a95c into master Jul 6, 2017
@rndstr rndstr deleted the table-overflow-plus-one branch July 6, 2017 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants