Allow pagination for interpreted results#481
Conversation
| pageNumber: 0, | ||
| numPages: numPagesOfResultSet(resultSet), | ||
| resultSet, | ||
| resultSet: { t: 'RawResultSet', ...resultSet }, |
There was a problem hiding this comment.
Has the type RawResultSet changed? In the current main branch, it looks like there is no t property.
There was a problem hiding this comment.
To accomodate sarif results, I changed the type of the .resultSet field of ExtensionParsedResultSets from being always only a RawResultSet to being a ResultSet (which is either a RawResultSet or a SarifResultSet, tagged appropriately)
| extends React.Component<ResultTablesProps, ResultTablesState> { | ||
|
|
||
| private getResultSets(): ResultSet[] { | ||
| private static _getResultSetsOfProps(props: ResultTablesProps): ResultSet[] { |
There was a problem hiding this comment.
So that it can be called from the static lifecycle method getDerivedStateFromProps.
| return resultSets; | ||
| } | ||
|
|
||
| private getResultSets(): ResultSet[] { |
There was a problem hiding this comment.
Looks like this isn't used.
Wait...it is. Though, not sure what the difference is between this function and the static _getResultSetsOfProps.
There was a problem hiding this comment.
I just factored out the body of that method into a static method that takes a props argument, so that I could call it from getDerivedStateFromProps.
| /** | ||
| * Holds if we actually should show pagination interface right now. This is | ||
| * still false for the time being when we're viewing alerts. | ||
| * true as long as the result sets we're given are marked to permit it. |
There was a problem hiding this comment.
should probably just get rid of this function since it duplicates paginationAllowed without adding anything new. And paginationAllowed will be removed when we remove the pagination flag, right?
There was a problem hiding this comment.
Sure, could just inline that now.
|
It looks like this adds pagination for alerts table, but not paths tables. Is that part coming later? |
No, it works for me on path tables when I test it? This may be a worthwhile cleanup in how things are named if that's confusing. |
|
OK...tried this out on a proper path query. Looks great. A couple of things, though:
|
|
I think for 2, the problem has to do wih |
aeisenberg
left a comment
aeisenberg
left a comment
There was a problem hiding this comment.
Nice job!
I have some time now, so I am going to look at a little better style.
2 participants
Enable pagination for interpreted as well as raw results.
This is still under feature flag, so no user-facing changes quite yet, but the flag can possibly be enabled for everyone once this lands. There will be a number of code cleanups available at that point.
#277