App Search - Polishing Analytics Views by daveyholler · Pull Request #92939 · elastic/kibana

daveyholler · 2021-02-25T23:35:46Z

Summary

Makes some visual adjustments to the Analytics views.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
~~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list~~
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

daveyholler · 2021-02-25T23:36:43Z

..._search/public/applications/app_search/components/analytics/components/analytics_section.tsx

+        {iconType ? (
+          <EuiFlexItem grow={false}>
+            <EuiIcon type={iconType} size="l" />
+          </EuiFlexItem>
+        ) : null}


Adds the ability for an icon to be passed into the header

daveyholler · 2021-02-25T23:37:53Z

...pplications/app_search/components/analytics/components/analytics_tables/analytics_table.scss

+.entSearch__table--noBackground {
+  .euiTable {
+    background-color: transparent;
+  }
+}


A bit of a workaround right now to use the tables on the subdued panels for amsterdam. Until this is officially supported in EUI, this should suffice.

daveyholler · 2021-02-25T23:39:05Z

...pplications/app_search/components/analytics/components/analytics_tables/inline_tags_list.tsx

    </EuiBadgeGroup>
  );
+
+  return displayCountOnly ? <TagToolTip /> : <TagList />;


This change introduces a way to render only a tooltip with a total tag count. Works well for cases where the tables are small and compact.

I'm a little confused by this because it looks like none of the tables are rendering a <TagList /> now, even though it feels like the RecentQueriesTable and the QueryClicksTable could:

From the code I think I see why and what we need to change, but just want to check with you first that we even still even want the <TagToolTip /> component - it would be much simpler code-wise to only have one way of displaying tags.

This should be fixed now in 5e1fc00:

daveyholler · 2021-02-25T23:41:35Z

...lugins/enterprise_search/public/applications/app_search/components/data_panel/data_panel.tsx

+    <EuiSpacer />
+    {children}
+  </EuiPanel>
+);


Adds a new component called DataPanel that supports rendering things like tables and charts with a title, subtitle, and action

daveyholler · 2021-02-25T23:42:48Z

...search/public/applications/app_search/components/engine_overview/components/total_charts.tsx

+        <DataPanel
+          data-test-subj="TotalQueriesChart"
+          title={TOTAL_QUERIES}
+          subtitle={LAST_7_DAYS}
+          action={
+            <EuiButtonEmptyTo
+              iconType="eye"
+              to={generateEnginePath(ENGINE_ANALYTICS_PATH)}
+              size="s"
+            >
+              {VIEW_ANALYTICS}
+            </EuiButtonEmptyTo>
+          }
+        >
+          <AnalyticsChart
+            lines={[
+              {
+                id: TOTAL_QUERIES,
+                data: convertToChartData({ startDate, data: queriesPerDay }),
+              },
+            ]}
+          />
+        </DataPanel>


Uses the new DataPanel component. Eventually (in later updates of Amsterdam) this will support 2 basic versions of panels... a subdued version and a bordered version. Charts will use the bordered version.

daveyholler · 2021-02-25T23:43:20Z

...search/public/applications/app_search/components/engine_overview/engine_overview_metrics.tsx

+          <EuiFlexGroup>
+            <EuiFlexItem grow={1}>
+              <TotalStats />
+            </EuiFlexItem>
+            <EuiFlexItem grow={3}>
+              <TotalCharts />
+            </EuiFlexItem>
+          </EuiFlexGroup>


Renders the stats and charts in a row, rather than stacked vertically.

Do we want to adjust/reduce the chart height the charts at all as a result? They look a little squashed on a laptop screen:

Also there's some extra space that the current height is causing on the stat cards which we could easily lose:

Example with a height={240} passed into both AnalyticsCharts in the TotalCharts component:

The above looks slightly more balanced to me - what do you think?

Good call 👍🏼

daveyholler · 2021-02-25T23:44:37Z

x-pack/plugins/enterprise_search/public/applications/app_search/icons/cursor_icon.tsx

@@ -0,0 +1,22 @@
+/*


Adds a new icon. This directory will also include our icons for engines and meta_engines and all other app search specific icons we'll have. (Like on the relevance tuning pages)

byronhulcher · 2021-03-01T16:46:42Z

Hey @daveyholler I'm going to take a look at this tomorrow (Tuesday)!

daveyholler · 2021-03-01T18:38:32Z

@elasticmachine merge upstream

cee-chen · 2021-03-01T23:17:01Z

@elasticmachine merge upstream

cee-chen · 2021-03-01T23:17:49Z

@daveyholler just FYI I think there was a space in the front of your comment which caused elasticmachine to ignore you, it's very picky that way 😬

...s/enterprise_search/public/applications/app_search/components/data_panel/data_panel.test.tsx

...pplications/app_search/components/analytics/components/analytics_tables/inline_tags_list.tsx

cee-chen · 2021-03-01T23:42:56Z

...pplications/app_search/components/analytics/components/analytics_tables/inline_tags_list.tsx

    </EuiBadgeGroup>
  );
+
+  return displayCountOnly ? <TagToolTip /> : <TagList />;


I'm a little confused by this because it looks like none of the tables are rendering a <TagList /> now, even though it feels like the RecentQueriesTable and the QueryClicksTable could:

From the code I think I see why and what we need to change, but just want to check with you first that we even still even want the <TagToolTip /> component - it would be much simpler code-wise to only have one way of displaying tags.

..._search/public/applications/app_search/components/analytics/components/analytics_section.tsx

x-pack/plugins/enterprise_search/public/applications/shared/icons/cursor.svg

x-pack/plugins/enterprise_search/public/applications/app_search/icons/cursor_icon.tsx

x-pack/plugins/enterprise_search/public/applications/app_search/icons/index.ts

...pplications/app_search/components/analytics/components/analytics_tables/analytics_table.scss

...cations/app_search/components/analytics/components/analytics_tables/analytics_table.test.tsx

...h/public/applications/app_search/components/engine_overview/components/total_charts.test.tsx

...lugins/enterprise_search/public/applications/app_search/components/data_panel/data_panel.tsx

cee-chen

I need to stop here for the night, my brain is pretty fried, but I still have DataPanel and tables/charts to look at - I'll continue reviewing tomorrow.

I do strongly want to say that I think this PR could have been broken up into much smaller and more atomic PRs and that would have personally made reviewing significantly easier for me in terms of QA and code review. For example, InlineTagsList could have been its own PR, DataPanel could have been its own PR, icons in AnalyticsHeader could have been separate, and so on and so forth.

I'd really appreciate discussing that approach more if we're going to continue doing polish PRs throughout the migration - smaller and more contained PRs are almost always going to be way more manageable and easier for reviewers to set aside time for, and I honestly think you'll have a better experience as well (in terms of getting help/review sooner, CI issues, and not having a ton of change requests per PR).

- Remove unused svg file - Add TODO comment - Add test coverage

TotalCharts - adjust chart height - tests: simplify / convert back to shallow RecentApiLogs - switch to DataPanel

Component - move CSS table bg override out from being an AnalyticsTable concern to a DataPanel concern + bem naming, todo comment - add responsive={false} for better mobile UX - add className and data-test-subj prop passing - change title to pass full heading tags rather than a string - move subtitle below title for better screen reader order - add index.ts export Tests - capitalization for consistency, ordering/describe - remove data-test-subjs on source code (simpler to grab & inspect tags directly so we can more easily call .text() on passed content) - add new tests for props (fliled/className/data-test-subj) Usage - update other files using DataPanel to start passing heading tags + use index export - fix RecentApiLogs tests - change RecentQueries section to use a DataPanel (per Davey)

Tags - Rename to more general tags.tsx file - Add CSS limiting width of variable length tags - Break up into two separate components for easier readability & testing - Split up tags column constants so that the wider tables can use the old tags list component Tables - add isSmall flag to AnalyticsTable to use new tag count component - reduce actions column width - revert unnecessary table test changes Analytics - add custom CSS that switches tables/panels into full-width earlier

- fix responsive behavior with icon - add missing AnalyticsSection branch coverage

cee-chen

Various responsive before/after screencaps

cee-chen · 2021-03-03T02:21:17Z

..._search/public/applications/app_search/components/analytics/components/analytics_section.tsx

+        gutterSize="xs"
+        alignItems="center"
+        justifyContent="flexStart"
+        responsive={false}


Before (no responsive={false}):

After:

cee-chen · 2021-03-03T02:21:31Z

...lugins/enterprise_search/public/applications/app_search/components/data_panel/data_panel.tsx

+
+  return (
+    <EuiPanel {...props} color={filled ? 'subdued' : 'plain'} className={classes} hasShadow={false}>
+      <EuiFlexGroup justifyContent="spaceBetween" alignItems="center" responsive={false}>


Before (no responsive={false}):

After:

cee-chen · 2021-03-03T02:28:11Z

...s/enterprise_search/public/applications/app_search/components/analytics/views/analytics.scss

+.analyticsOverviewTables {
+  @include euiBreakpoint('xs', 's', 'm', 'l') {
+    flex-direction: column; // Force full width on table panels earlier to ensure content stays legible
+  }
+}


Before:

After:

cee-chen · 2021-03-03T02:37:22Z

@daveyholler That's the last of my changes for now! Thanks for letting me push directly to your branch - hopefully this was a faster experience for both of us :) Please do pull down the latest commits and test them out locally and let me know if everything looks good to you or if you see any issues. I've tested at various responsive widths and am fairly satisfied the new changes look functional on all of them.

If any specific changes in any commits look odd to you or you're not sure why a change was made, definitely let me know and I'm happy to dive into it!

kibanamachine · 2021-03-03T03:58:50Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1322	1353	+31

Async chunks

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

id	before	after	diff
`enterpriseSearch`	2.1MB	2.1MB	+14.8KB
`triggersActionsUi`	1.6MB	1.5MB	-23.9KB
total			-9.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	104.0KB	104.1KB	+82.0B

Unknown metric groups

async chunk count

id	before	after	diff
`triggersActionsUi`	41	42	+1

History

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

daveyholler · 2021-03-03T17:18:30Z

@constancecchen this stuff looks so good. Thanks a million for helping out.

@byronhulcher FYI: The DataPanel component we chatted about will land with this PR.

cee-chen · 2021-03-03T17:31:48Z

No problem! Let me know whenever you've pulled down and vetted/QA's all the changes and if you don't see any issues/change requests, I can approve and we can merge after if that sounds good

cee-chen

This looked good just now during demos, so I'm gonna go ahead and approve

cee-chen · 2021-03-03T20:24:37Z

@daveyholler shoot, I forgot to remind you to add the auto-backport label - without it, you'll have to manually run yarn backport and babysit/merge your backport PR. LMK if I can answer any q's about that!

* WIP analytics polish * Working on tests. * Finishing up tests. * Updating the engine overview page. * eslint fixes * i18n fixes * Icons feedback - Remove unused svg file - Add TODO comment - Add test coverage * linting - unnecessary ternaries * EnginesOverview feedback TotalCharts - adjust chart height - tests: simplify / convert back to shallow RecentApiLogs - switch to DataPanel * DataPanel feedback Component - move CSS table bg override out from being an AnalyticsTable concern to a DataPanel concern + bem naming, todo comment - add responsive={false} for better mobile UX - add className and data-test-subj prop passing - change title to pass full heading tags rather than a string - move subtitle below title for better screen reader order - add index.ts export Tests - capitalization for consistency, ordering/describe - remove data-test-subjs on source code (simpler to grab & inspect tags directly so we can more easily call .text() on passed content) - add new tests for props (fliled/className/data-test-subj) Usage - update other files using DataPanel to start passing heading tags + use index export - fix RecentApiLogs tests - change RecentQueries section to use a DataPanel (per Davey) * Analytics tags - updates & responsive tweaks Tags - Rename to more general tags.tsx file - Add CSS limiting width of variable length tags - Break up into two separate components for easier readability & testing - Split up tags column constants so that the wider tables can use the old tags list component Tables - add isSmall flag to AnalyticsTable to use new tag count component - reduce actions column width - revert unnecessary table test changes Analytics - add custom CSS that switches tables/panels into full-width earlier * AnalyticsSection fixes - fix responsive behavior with icon - add missing AnalyticsSection branch coverage Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen.3@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Constance Chen <constance.chen.3@gmail.com>

* master: (48 commits) Fix wrong import in data plugin causing 100kB bundle increase (elastic#93448) [Fleet] Correctly track install status of an integration (elastic#93464) Reviews data frame analytics UI text (elastic#93033) Display multiple copyable fields for process.args in resolver node detail panel (elastic#93280) [Security Solution][Detections] ML Popover overflow fix (elastic#93525) chore(NA): do not use execa on bazel workspace status update script (elastic#93532) Bump dependencies (elastic#93511) [dev/build_ts_refs] support disabling the ts-refs build completely (elastic#93529) [Security Solution] fix data provider cypress test (elastic#93465) Fix service map for All environment single service (elastic#93517) [Fleet] Fix package version comparaison in the UI (elastic#93498) [alerting] adds doc on JSON-expanded action variables and task manager max_workers (elastic#92720) [dev/build_ts_refs] ignore type checking failures when building ts refs (elastic#93473) [core-new-docs] Adds a dev-doc for core documentation (elastic#92976) remove opacity from maps legacy style (elastic#93456) [Security Solution][Lists] Escape quotes in list ids and quote the id in KQL query (elastic#93176) Revert "Make tests deterministic by providing unique timestamps (elastic#93350)" [Discover] Fix link from dashboard saved search to Discover (elastic#92937) update public api docs App Search - Polishing Analytics Views (elastic#92939) ...

daveyholler added 4 commits February 25, 2021 14:39

WIP analytics polish

d54f566

Working on tests.

ea28e03

Finishing up tests.

00cdaf5

Updating the engine overview page.

9ae5a3d

daveyholler commented Feb 25, 2021

View reviewed changes

daveyholler marked this pull request as ready for review February 25, 2021 23:45

daveyholler requested review from a team February 25, 2021 23:45

daveyholler added release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Feb 25, 2021

byronhulcher self-requested a review March 1, 2021 16:46

Merge branch 'master' into daveyholler/as-analytics-polish

79b8923

cee-chen reviewed Mar 1, 2021

View reviewed changes

...s/enterprise_search/public/applications/app_search/components/data_panel/data_panel.test.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Mar 1, 2021

View reviewed changes

..._search/public/applications/app_search/components/analytics/components/analytics_section.tsx Outdated Show resolved Hide resolved

cee-chen reviewed Mar 1, 2021

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/shared/icons/cursor.svg Outdated Show resolved Hide resolved

cee-chen reviewed Mar 1, 2021

View reviewed changes

x-pack/plugins/enterprise_search/public/applications/app_search/icons/cursor_icon.tsx Show resolved Hide resolved