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

[CTI] shortens large numbers on Dashboard Link Panel #105269

Merged
merged 3 commits into from
Jul 13, 2021

Conversation

ecezalp
Copy link
Contributor

@ecezalp ecezalp commented Jul 12, 2021

Summary

  • For large numbers, adds K M B as appropriate

Screen Shot 2021-07-12 at 1 11 57 PM

  • updates Showing: ${x} events to Showing: ${x} indicators

Checklist

@ecezalp ecezalp added release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed Team: CTI labels Jul 12, 2021
@ecezalp ecezalp requested a review from a team July 12, 2021 17:17
@ecezalp ecezalp self-assigned this Jul 12, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

I appreciate this work being broken out into a common helper with tests 👍 . We may be able to leverage the @elastic/numeral package to reduce some of the complexity here, but behavior-wise this LGTM.

*/
export const shortenCountIntoString = (count: number): string => {
if (count < 10000) {
return count.toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use .toLocaleString here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually for numbers under 10000 I believe we don't want to see 9,999 vs 9.999, just 9999 @yiyangliu9286

if (count < 10000) {
return count.toString();
}
const si = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: there are a lot of one- or two-character variables in this function; more declarative names might help with legibility here. E.g. magnitude and unit instead of v and s

Comment on lines 26 to 31
let i;
for (i = si.length - 1; i > 0; i--) {
if (count >= si[i].v) {
break;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could reverse the above array to simplify this logic:

Suggested change
let i;
for (i = si.length - 1; i > 0; i--) {
if (count >= si[i].v) {
break;
}
}
const { magnitude, unit } = abbreviations.find((abbreviation) => count >= abbreviation.magnitude);

}

return (
toFixedWithoutRounding(count / si[i].v, 1).replace(/\.0+$|(\.[0-9]*[1-9])0+$/, '$1') + si[i].s
Copy link
Contributor

Choose a reason for hiding this comment

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

The @elastic/numeral package might be able to do some of this formatting for you, if you haven't yet explored that option.

@ecezalp ecezalp enabled auto-merge (squash) July 12, 2021 19:34
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2206 2207 +1

Async chunks

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

id before after diff
securitySolution 6.3MB 6.3MB +637.0B

History

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

cc @ecezalp

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 13, 2021
Co-authored-by: Ece Özalp <ozale272@newschool.edu>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 13, 2021
…-png-pdf-report-type

* 'master' of github.com:elastic/kibana: (292 commits)
  bring back KQL autocomplete in timeline + fix last updated (elastic#105380)
  [Maps] Change TOC pop-up wording to reflect filter change, not search bar change (elastic#105163)
  Updating urls to upstream elastic repo (elastic#105250)
  [Maps] Move new vector layer wizard card down (elastic#104797)
  Exclude registering the cases feature if not enabled (elastic#105292)
  [Uptime] Alerts - Monitor status alert - check monitor status by monitor.timespan (elastic#104541)
  updated UI copy (elastic#105184)
  Log a warning when documents of unknown types are detected during migration (elastic#105213)
  [Logs UI] Register log threshold rule as lifecycle rule (elastic#104341)
  [Ingest pipelines] add network direction processor (elastic#103436)
  [Console] Autocomplete definitions (manual backport) (elastic#105086)
  [Security Solution] User can make Exceptions for Memory protection alerts (elastic#102196)
  [Lens] Formula: add validation for multiple field/metrics (elastic#104092)
  Removing async from file upload and data visualizer plugins start lifecycle (elastic#105197)
  Fix error when validating the form with non blocking validations (elastic#103629)
  [ML] Fix "View by" swim lane with applied filter and sorting by score  (elastic#105217)
  Update dependency @elastic/charts to v32 (elastic#104625)
  [CTI] shortens large numbers on Dashboard Link Panel (elastic#105269)
  [Security Solution][Endpoint][Host Isolation] Fixes bug to remove excess host metadata status toasts on non user initiated errors (elastic#105331)
  [Cases] Fix pushing alerts count on every push to external service (elastic#105030)
  ...

# Conflicts:
#	x-pack/plugins/reporting/common/types.ts
ecezalp added a commit to ecezalp/kibana that referenced this pull request Jul 19, 2021
# Conflicts:
#	x-pack/plugins/security_solution/public/overview/components/overview_cti_links/threat_intel_panel_view.tsx
ecezalp added a commit that referenced this pull request Jul 20, 2021
# Conflicts:
#	x-pack/plugins/security_solution/public/overview/components/overview_cti_links/threat_intel_panel_view.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: CTI Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants