-
Notifications
You must be signed in to change notification settings - Fork 16
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
Table Widget: core functionality #154
Conversation
Pull Request Test Coverage Report for Build 1648975353
💛 - Coveralls |
8285a99
to
0abfb59
Compare
b8b4015
to
c86a159
Compare
037d57e
to
503664b
Compare
component='div' | ||
count={totalCount} | ||
rowsPerPage={rowsPerPage} | ||
page={page - 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to make this adjustment because TablePagination
from MUI is zero-based whereas our pagination API is 1-based (see https://github.com/CartoDB/carto-react/blob/master/packages/react-workers/src/workers/features.worker.js#L166)
Or maybe we could change our own implementation to start at page 0 too? Would it be possible @Clebal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbecquet totally agree, it's a mistake that page is 1-based. Sorry for my late response, I didn't saw this. Can you open a PR with that change?
187e8bf
to
913318c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks really web but I'm wondering about the behavior when we change the page and after that, the data changes because the viewport has changed or we've applied a filter. In that case, we could be consulting a page that no longer exists and the table would show no data.
@alasarr, @borja-munoz we need to talk about what to do in that case (because is a very common case). Probably, we would need to go back to the first page every time that the data changes.
913318c
to
1984724
Compare
Looks good to me for a first version. Remember to update the changelog. |
@padawannn about your comment on pagination vs. viewport, I've added this piece of code 17454dd which resets the page to 1 when the features change. |
@bbecquet we also need to reset the page to 1 if filters change. For example, in your branch template when you click hover a category in the category widget. We also should reset the page to 1 if the datasource change although this case is not common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6061fd3
to
9fac11e
Compare
@padawannn Thanks for the explanation about filters. I've changed the reset callback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bbecquet you forgot to include the dataSource
as a reset condition although it is not a common case.
On the other hand, it could be great if we add some tests about page reset
8365668
to
0b128b9
Compare
Screenshot from integration in carto-react-template sample app 3
This PR introduces
TableWidgetUI
as a new UI component andTableWidget
as a new widget.For now, supports sorting, pagination and click on row behavior.
Selection, search and export will be managed in further PRs.
TableWidgetUI
behavior is fully controlled, so filtering, sorting and pagination has to be done by the parent component. This allowsTableWidget
to use the newgetRawFeatures
API from #225, to access and sort features in an efficient way.