[Cloud Posture] add pagination to resource findings table#131422
[Cloud Posture] add pagination to resource findings table#131422orouz merged 8 commits intoelastic:mainfrom
Conversation
e0d5587 to
68cbbc5
Compare
|
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security Posture) |
|
reject fast: having 11 results, page size of 10. the second page shows another 10 instead of 1 result. Attaching video: Screen.Recording.2022-05-04.at.11.46.45.mov |
...pages/findings/latest_findings_by_resource/resource_findings/resource_findings_container.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/cloud_security_posture/public/pages/findings/utils.ts
Outdated
Show resolved
Hide resolved
kfirpeled
left a comment
There was a problem hiding this comment.
see attached video, pagination is not working well. from calculation is not done properly
58c0527 to
77b7b84
Compare
77b7b84 to
c87fada
Compare
c87fada to
1615bf0
Compare
...s/cloud_security_posture/public/pages/findings/latest_findings/latest_findings_container.tsx
Show resolved
Hide resolved
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
JordanSh
left a comment
There was a problem hiding this comment.
looks good, some minor suggestions
| const LatestFindingsPageTitle = () => ( | ||
| <PageTitle> | ||
| <PageTitleText | ||
| title={<FormattedMessage id="xpack.csp.findings.findingsTitle" defaultMessage="Findings" />} | ||
| /> | ||
| </PageTitle> | ||
| ); |
There was a problem hiding this comment.
nit but I'm not sure why this static component needs to be separated. I would just leave it inside the page component
There was a problem hiding this comment.
probably subjective, but IMO it makes the root component read nicer as we don't look at long lines that aren't really interesting. LatestFindingsPageTitle tells me all i need to know without looking how it's done
| <Link to={generatePath(findingsNavigation.findings_by_resource.path)}> | ||
| <EuiButtonEmpty iconType={'arrowLeft'}> | ||
| <FormattedMessage | ||
| id="xpack.csp.findings.resourceFindings.backToResourcesPageButtonLabel" | ||
| defaultMessage="Back to group by resource view" | ||
| /> | ||
| </EuiButtonEmpty> | ||
| </Link> |
There was a problem hiding this comment.
do we actually need both EuiButton and react-router-dom Link? consider doing:
const BackToResourcesButton = () => {
const { push } = useHistory();
return (
<EuiButtonEmpty
iconType={'arrowLeft'}
onClick={() => push(generatePath(findingsNavigation.findings_by_resource.path))}
>
<FormattedMessage
id="xpack.csp.findings.resourceFindings.backToResourcesPageButtonLabel"
defaultMessage="Back to group by resource view"
/>
</EuiButtonEmpty>
);
};
Eui don't mind using buttons as links and the opposite aswell
is there a way to do a push with href?
There was a problem hiding this comment.
i agree it's unfortunate, but IMO better than using onClick, which makes it a non-anchor link. this means you can't:
- copy the link
ctrl+clickthe link to open in a new tab
what i'd like to do is use href in EuiButtonEmpty but it doesn't work with react-router
there's some info on this here but i didn't manage to make anything out of it.
Summary
this PR adds pagination using
fromandsize