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

[TablePagination] Out of range warning when "count={-1}" #19874

Merged

Conversation

dbarabashh
Copy link
Contributor

@dbarabashh dbarabashh commented Feb 27, 2020

Hey,

I've added changes for TablePagination component if count value negative issue. Also, we should add a test interacting with the pagination icons whencount={-1}, as @oliviertassinari wrote. We can wait for test and merge this PR or merge this one only with my changes.

P.S I can add test but a bit later too :)

Closes #19865

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 27, 2020

No bundle size changes comparing 0033bb3...76d5dbf

Generated by 🚫 dangerJS against 76d5dbf

@oliviertassinari
Copy link
Member

Thanks for taking the time.

I don't think that a new test for this warning is something we should add on its own. However, I think that it tips us that we miss test coverage for a more important feature: the icons of the table pagination should work correctly with count={-1}. If we had such a test, the CI would have caught the warning by default (fail). So we definitely miss a test case :)

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module! PR: needs test The pull request can't be merged PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: needs test The pull request can't be merged labels Feb 27, 2020
@dbarabashh
Copy link
Contributor Author

I messed some things, let me continue work on that

@dbarabashh
Copy link
Contributor Author

@oliviertassinari , check it out, please

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 27, 2020
@dbarabashh
Copy link
Contributor Author

@oliviertassinari @eps1lon Work in Progress :) sorry

@oliviertassinari
Copy link
Member

@dbarabashdev I have updated the test with the idea I had in mind. What do you think? Does it match the expectation you had? Thanks :)

@dbarabashh
Copy link
Contributor Author

@oliviertassinari ahhh, yup, right I understood what do you mean

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Sweet :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: table This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TablePagination] Out of range warning when "count={-1}"
4 participants