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

[Cloud Posture] Dashboard Redesign - data counter cards #144565

Merged
merged 17 commits into from
Nov 9, 2022

Conversation

JordanSh
Copy link
Contributor

@JordanSh JordanSh commented Nov 3, 2022

Summary

Resolves #143865
Resolves #143857

Added new counter components based on the dashboard redesign, full feature list can be found in the mentioned tasks

  • resources counter navigates to the findings by resource table
  • failing findings counter navigates to the findings table filtered by failed findings
Screen.Recording.2022-11-03.at.20.31.56.mov
  • numbers above 1,000,000 are formatted using compact abbreviation
  • if there are 0 failed findings, the counter will not be red

image

  • using the cardinality aggregation to get the number of unique resource ids
const uniqueResourcesCountQuery = {
  resources_evaluated: {
    cardinality: {
      field: 'resource.id',
    },
  },
};

@JordanSh JordanSh added release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related labels Nov 3, 2022
@JordanSh JordanSh self-assigned this Nov 3, 2022
@JordanSh JordanSh marked this pull request as ready for review November 7, 2022 17:24
@JordanSh JordanSh requested a review from a team as a code owner November 7, 2022 17:24
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security Posture)

Comment on lines 19 to 25
return <span>{number.toLocaleString('en-US')}</span>;
}

return (
<EuiToolTip content={number.toLocaleString('en-US')}>
<span>
{number.toLocaleString('en-US', {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Using hard-coded locale is problematic. I suggest still using FormattedNumber and only passing the notation prop if the number is large enough.
  2. You changed logic for existing usages of CompactFormattedNumber - please make sure this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yeah i forgot to remove the locales argument.

<FormattedNumber> does not work, thats why we didn't use it in the findings either.
the findings page use @elastic/numeral instead, but its API is limited and didn't match the formats used in the designs or our previous formatting. i would argue it should be switched to use this component as well instead.

I'll remove the locales parameter, when its undefined the host's locale will be used. ty for the catch

export type CspCounterCardProps = Pick<EuiCardProps, 'onClick' | 'id'> &
Pick<EuiStatProps, 'title' | 'description' | 'titleColor' | 'isLoading'>;

export const CspCounterCard = ({ counter }: { counter: CspCounterCardProps }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for a container counter object - I suggest omitting the top level counter prop and simply accept here the "internal" props (i.e. onClick, id, title, etc)

@@ -56,9 +63,60 @@ export const CloudSummarySection = ({
navToFindings({ 'result.evaluation': RULE_FAILED });
};

const counters: CspCounterCardProps[] = useMemo(
Copy link
Contributor

@ari-aviran ari-aviran Nov 8, 2022

Choose a reason for hiding this comment

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

Consider just writing this in the declarative JSX instead of creating a variable and then using it:

<EuiFlexItem key={DASHBOARD_COUNTER_CARDS.CLUSTERS_EVALUATED}>
  <CspCounterCard
    counter={{
      id: DASHBOARD_COUNTER_CARDS.CLUSTERS_EVALUATED,
      title: <CompactFormattedNumber number={complianceData.clusters.length} />,
      description: i18n.translate(
        'xpack.csp.dashboard.summarySection.counterCard.clustersEvaluatedDescription',
        { defaultMessage: 'Clusters Evaluated' }
      ),
    }}
  />
</EuiFlexItem>
/* Rest of the cards */

I prefer it this way since you immediately "see" the layout when you look at the JSX, instead of jumping back and forth between counters definition and the JSX

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess its subjective, I prefer to have the JSX as clean as possible. "seeing" the layout is very important to me as well, I find it easier when things are clean and compact, I'm looking at it as a "list of counter cards" and keep the properties mess outside

Comment on lines 28 to 30
.euiCard__title {
display: none;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hiding the title with CSS is hacky, I suggest instead to utilize the EuiCard title and children props properly and completely dump EuiStat:

export const CspCounterCard = ({ counter }: { counter: CspCounterCardProps }) => {
  const { euiTheme } = useEuiTheme();

  return (
    <EuiCard
      title={
        <EuiTitle size="xxxs">
          <h6>{counter.description}</h6>
        </EuiTitle>
      }
      hasBorder
      onClick={counter.onClick}
      paddingSize="m"
      layout="horizontal"
      css={css`
        position: relative;

        :hover .euiIcon {
          color: ${euiTheme.colors.primary};
          transition: ${euiTheme.animation.normal};
        }
      `}
      data-test-subj={counter.id}
    >
      <EuiText color={counter.titleColor}>
        <EuiTitle size="xs">
          <h3>{counter.title}</h3>
        </EuiTitle>
      </EuiText>
      {counter.onClick && (
        <EuiIcon
          type="link"
          css={css`
            position: absolute;
            top: ${euiTheme.size.m};
            right: ${euiTheme.size.m};
          `}
        />
      )}
    </EuiCard>
  );
};

In addition, I suggest flipping title and description so that title will be the actual title of the card.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great change, thanks!


render(
<TestProvider
core={{
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need this core override

},
}}
>
{/* <ComplianceDashboard />*/}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

Comment on lines 19 to 21
beforeEach(() => {
jest.resetAllMocks();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to reset the mocks, you are not mocking anything in the tests here

import { TestProvider } from '../../../test/test_provider';
import { screen } from '@testing-library/react';

describe('<CloudSummarySection />', () => {
Copy link
Contributor

@ari-aviran ari-aviran Nov 8, 2022

Choose a reason for hiding this comment

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

I'd expect tests here to verify the rest of the functionality of the section (risks table, score chart, etc.) If you consider this out of scope please open a task

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i'll add those tests here on every new component task, its already listed in the tasks

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
cloudSecurityPosture 127 128 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 128.0KB 129.9KB +1.9KB
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 108 113 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 109 115 +6
securitySolution 517 523 +6
total +20

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @JordanSh


export const CspCounterCard = ({ counter }: { counter: CspCounterCardProps }) => {
export const CspCounterCard = (counter: CspCounterCardProps) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider de-structuring props in the arguments: ({ title, onClick, id, description, descriptionColor })

hasBorder
onClick={counter.onClick}
paddingSize="m"
textAlign="left"
layout="vertical"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the default and can be omitted as we don't really care about the layout since we don't use an icon, we only care about the text alignment

@JordanSh JordanSh merged commit 1ff9ceb into elastic:main Nov 9, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 9, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 9, 2022
* main:
  [Lens] Rearrange options (elastic#144891)
  [Actionable Observability] Integrate alert search bar on rule details page (elastic#144718)
  [Security Solution] [Exceptions] Adds options to create a shared exception list and to create a single item from the manage exceptions view (elastic#144575)
  [Actionable Observability] Add context.alertDetailsUrl to connector template for Uptime > Monitor status & Uptime TLS rules (elastic#144740)
  [Security Solution] [Feat] Add Bulk Events to Timeline. (elastic#142737)
  [TIP] Env specific cypress config (elastic#144894)
  skip flaky suite (elastic#144885)
  [Enterprise Search] Fixes Search Index page to go blank when connection lost (elastic#144022)
  [Cloud Posture] track findings pages (elastic#144822)
  [ContentManagement] Inspector flyout (elastic#144240)
  [Cloud Posture] Dashboard Redesign - data counter cards (elastic#144565)
  [TIP] Run e2e pipeline on CI (elastic#144776)
  [Guided onboarding] Config updates for the Security guide (elastic#144844)
  Cleanup unused code for claiming tasks by id (elastic#144408)
  Ping the response-ops team whenever a new connector type is registered (elastic#144736)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Cloud Security Cloud Security team related v8.6.0
Projects
None yet
5 participants