[App Search] Add final Analytics table components

[cee-chen]

Summary

Adds the final Analytics tables and completes the queries views. Components added:

• Specific views:
  • AnalyticsTable (+ one-offs: RecentQueriesTable, QueryClicksTable)
  • AnalyticsSearch (a search bar that sends users directly to the Query Detail view)
• Supporting components:
  • InlineTagsList
  • AnalyticsSection

I hadn't thought this was going to be as much code as it ended up being (1k+ lines 💀) - I definitely recommend following along by-commit as I tried to break this up as granularly as possible.

Screenshots

Analytics overview:

Empty state:

Query detail view:

Empty state:

Queries view - e.g., Top Queries, Recent Queries:

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility

[JasonStoltz]

This is fine, just asking out of curiosity. Why use Query['tags'] here instead of just string[].

[cee-chen]

@byronhulcher got me on this habit recently! #88095 (comment)

I like the idea of it because it means if we ever change a type/data structure in the future we can update it in one place and all other places that we're just passing that same prop to get it automatically.

[JasonStoltz]

It's also more explicit about what specifically should be passed, which helps with understanding I think.

[JasonStoltz]

LGTM, no concerns

[JasonStoltz]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 7e3044e

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
enterpriseSearch	1172	1189	+17

Async chunks

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

id	before	after	diff
enterpriseSearch	1.8MB	1.8MB	+21.6KB

History

• 💔 Build #101824 failed 8a95ff0
• 💚 Build #100944 succeeded ef008d9
• 💔 Build #100937 failed 20811cc
• 💚 Build #100896 succeeded 6200a79

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

[byronhulcher]

Some minor comments. This looks good though, and I think it's fine to merge as is.

[byronhulcher]

I'm a big fan of using consts for pass-through values

[cee-chen]

I'm not. I've mentioned this before, and will continue to do so, but I very strongly prefer using static output and not variables in our test assertions.

1. Performance is typically a non-issue in tests. Developer understanding is paramount and (in my opinion) is the foremost goal when it comes to writing unit tests. Another developer should be able to read only your test file and fully understand what the source code does, why, and its edge cases.

2. With that understanding in mind, tests should be written for human readability and not for either compilation time, abstraction, or ease of updating. If your source code changes, your tests should typically change as well, as should documentation.

Consider the developer experience of the following:

const expectedValue = 'supercalifragilisticexpialidocious';

// …

// … a bunch of lines between your abstracted var and the test

// …

it('has an expected value', () => {
  expect(output).toEqual(expectedValue);
});

You're asking a developer to either hold in their memory the expected value between its declaration and it being used or you're asking them to find/scroll up and down your file. My question is: why? Why have this extra intermediate step or friction instead of assertions that read clearly and explicitly as-is, i.e.:

expect(output).toEqual('supercalifragilisticexpialidocious');

Now, consider the following worse example (it's contrived, but hopefully illustrative):

describe('someFn', () => {
  it('capitalizes strings and appends the word YOLO', () => {
    const input = 'supercalifragilisticexpialidocious';
    expect(someFn(input)).toEqual(input.toUpperCase() + 'YOLO');
  });
});

I consider this a particularly bad antipattern because the developer is literally just rewriting the logic of the function. That is NOT the point of a unit test, it doesn't scale the more complex the logic gets, and worse, it does not aid developer understanding (you might as well just go read the source code instead of reading the test). Consider this static example:

describe('someFn', () => {
  it('capitalizes strings and appends the word YOLO', () => {
    expect(
      someFn('supercalifragilisticexpialidocious')
    ).toEqual(
      'SUPERCALIFRAGILISTICEXPIALIDOCIOUSYOLO'
    );
  });
});

This unit test instead provides a good example of expected input/output and in combination with the test description, clearly illustrates (instead of duplicating) the source code.

Just to wrap up, I'm not saying we should never use variables or abstractions in tests. We absolutely do so in places and times where it makes sense (typically when it doesn't hinder explicit developer understanding). But I will strongly push for starting with static assertions as a baseline and abstractions as exception and not a rule in our unit tests.

[byronhulcher]

Is it okay to write props directly inside the React.FC<>?

[cee-chen]

I mean, it's okay in that Typescript isn't throwing an error on it and it's validating correctly :) It's not common in our current codebase but IMO this is essentially the same as doing this:

const someFn = ({ someObjValue }: { someObjValue: string }) => {}

as opposed to

interface SomeFn {
  someObjValue: string;
}
const someFn = ({ someObjValue }: SomeFn) => {}

They're both the same/valid Typescript, and depending on the complexity of the incoming params it's nicer to do an inline interface over pulling it out.

[byronhulcher]

Worth adding a note or something about why this custom CSS is applied (since we largely try to avoid it?)

[cee-chen]

I'm a little on the fence about it, we haven't added any comments so far on our CSS or previously had a history of doing so, so I'd probably want to consider it from a whole-app POV and discuss the benefits of doing so. I think so far the understanding is custom CSS consists mostly of EUI/layout overrides and hopefully it's easy to see what the CSS actually accomplishes by unchecking/hiding the custom CSS in a live browser

[cee-chen]

Thanks Jason & Byron for the thoughtful review - will merge this PR as-is, but definitely happy to continue any discussions or larger implications here or in the frontend channel!

[kibanamachine]

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.