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

Render every row on dashboard tables #1211

Merged
merged 3 commits into from Aug 16, 2016

Conversation

Calvinp
Copy link
Contributor

@Calvinp Calvinp commented Aug 12, 2016

Renders every row on the My Paused Requests and My Starred Requests tables on the dashboard page.

Note that this will render every row even if there are thousands - but there won't be thousands unless a user stars every request in the system for some reason.

cc @tpetr @wolfd @kwm4385

@@ -452,6 +452,7 @@ UITable.propTypes = {
keyGetter: PropTypes.func.isRequired,
children: PropTypes.arrayOf(PropTypes.node).isRequired,
paginated: PropTypes.bool,
isTruelyInfinite: PropTypes.bool, // Will render all rows even if there are 1,000,000
Copy link
Contributor

@tpetr tpetr Aug 15, 2016

Choose a reason for hiding this comment

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

can we think of a better name for this prop type? we shouldn't be depending on comments to explain what a prop does. furthermore, why cant paginated set to false support our use case here?

Copy link
Contributor Author

@Calvinp Calvinp Aug 15, 2016

Choose a reason for hiding this comment

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

I'll respond to the second point first. The problem is there are now three pagination options, so a boolean can't encompass them all:

  • Paginated (this can be split into api paginated and local paginated, but that's not relevant here)
  • Not paginated, but uses infinite scrolling (which is what the requests and tasks tables do)
  • Not paginated, renders all rows no matter what (which is introduced in this PR and is what the My Starred Requests table will do)

The third pagination type is implemented because we want to render all starred requests for ctrl-f access instantly upon loading the page. This won't happen if the user has more than a certain number of starred requests if we're using infinite scrolling.

So to be more descriptive with the name, maybe renderAllRowsImmediately or something like that would be a better name.

Copy link
Contributor

@tpetr tpetr Aug 15, 2016

Choose a reason for hiding this comment

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

Thanks for the clarification -- let's go with renderAllRows

@@ -452,6 +452,7 @@ UITable.propTypes = {
keyGetter: PropTypes.func.isRequired,
children: PropTypes.arrayOf(PropTypes.node).isRequired,
paginated: PropTypes.bool,
renderAllRows: PropTypes.bool, // Will render all rows even if there are 1,000,000
Copy link
Contributor

@tpetr tpetr Aug 16, 2016

Choose a reason for hiding this comment

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

comment is unnecessary now

@tpetr
Copy link
Contributor

tpetr commented Aug 16, 2016

LGTM after last piece of feedback

@Calvinp Calvinp merged commit 82ac507 into decaf Aug 16, 2016
@Calvinp Calvinp deleted the make_dashboard_pages_truely_infinite branch Aug 16, 2016
@tpetr tpetr modified the milestone: 0.10.0 Aug 18, 2016
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.

None yet

2 participants