-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Cloud Security] Add description list to findings flyout and copy buttons #184054
[Cloud Security] Add description list to findings flyout and copy buttons #184054
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
@einatjacoby , as we agreed |
`} | ||
> | ||
<CspInlineDescriptionList | ||
testId={FINDINGS_VULNERABILITY_FLYOUT_DESCRIPTION_LIST} |
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.
this test_id is imported from vulnerabilities, I think we should use a test id different for misconfigurations
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.
overall looks good just proposed some small changes
[ | ||
finding.resource?.id && { | ||
title: i18n.translate( | ||
'xpack.csp.vulnerabilities.vulnerabilitiesFindingFlyout.flyoutDescriptionList.resourceId', |
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 translations are using vulnerability identifiers, think we can use the identifiers that were being used before, i.e
xpack.csp.findings.findingsFlyout.overviewTab.resourceIdTitle
} from '@elastic/eui'; | ||
import { assertNever } from '@kbn/std'; | ||
import { i18n } from '@kbn/i18n'; | ||
import type { HttpSetup } from '@kbn/core/public'; | ||
import { generatePath } from 'react-router-dom'; | ||
import { css } from '@emotion/react/dist/emotion-react.cjs'; |
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.
import { css } from '@emotion/react/dist/emotion-react.cjs'; | |
import { css } from '@emotion/react'; |
…c-list-to-findings-flyout
Thanks @opauloh, all review comments were fixed |
…/JordanSh/kibana into add-desc-list-to-findings-flyout
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.
Just a single question if the change is showing a React key error in the console.
I had to make a similar change in the past and found that Flex was the issue, setting it to flex: 1
resolved it.
.long-and-truncated {
flex: 1;
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
}
title: `${item.title}:`, | ||
description: | ||
typeof item.description === 'string' ? ( | ||
<span> |
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.
Is this showing a React key warning?
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.
I'm not sure I follow Sean, is this regarding the CSS which i ended up not implementing in the end? let's take this offline so i can understand better
…c-list-to-findings-flyout
💚 Build Succeeded
Metrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @JordanSh |
Summary
Part of https://github.com/elastic/security-team/issues/9520 quick wins task
Screen.Recording.2024-05-22.at.21.14.52.mov
I also wanted to implement the ellipsis as shown in this figma but i tried various method like using
<EuiTextTruncate />
, using `className="eui-textTruncate", and just plain custom css for ellipsis. all of them are just not working very good with the icon at the end combined with the inline display. its probably possible but after trying many combinations i've decided that its not worth the time investment at the moment, if anyone has any suggestions i'd be hear of course!