[Rollup] Metrics multiple selection #42927

jloleysens · 2019-08-08T11:44:26Z

Summary

Addresses this issue.

Screenshots

Demo (first iteration)

Demo (latest iteration)

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
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
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…lection working

…r menu

elasticmachine · 2019-08-08T12:41:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-08T18:16:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-08-09T14:39:25Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…p-metrics-selectall * 'master' of github.com:elastic/kibana: (306 commits) [ML] Adding job overrides to the module setup endpoint (elastic#42946) [APM] Fix missing RUM url (elastic#42940) close socket timeouts without message (elastic#42456) Upgrade elastic/charts to 8.1.6 (elastic#42518) [ML] Delete old AngularJS data visualizer and refactor folders (elastic#42962) Add custom formatting for Date Nanos Format (elastic#42445) [Vega] Shim new platform - vega_fn.js -> vega_fn.js , use ExpressionFunction (elastic#42582) add socket.getPeerCertificate to KibanaRequest (elastic#42929) [Automation] ISTANBUL PRESET PATH is not working fine with constructor(private foo) (elastic#42683) [ML] Data frames: Updated stats structure. (elastic#42923) [Code] fixed the issue that the repository can not be deleted in some cases. (elastic#42841) [kbn-es] Support for passing regex value to ES (elastic#42651) Connect to Elasticsearch via SSL when starting kibana with `--ssl` (elastic#42840) Add Elasticsearch SSL support for integration tests (elastic#41765) Fix duplicate fetch in Visualize (elastic#41204) [DOCS] TSVB and Timelion clean up (elastic#42953) [Maps] [File upload] Fix maps geojson upload hanging on index step (elastic#42623) [APM] Use rounded bucket sizes for transaction distribution (elastic#42830) [yarn.lock] consistent resolve domain (elastic#42969) [Uptime] [Test] Repurpose unit test assertions to avoid flakiness (elastic#40650) ...

elasticmachine · 2019-08-09T15:58:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…p-metrics-selectall * 'master' of github.com:elastic/kibana: (22 commits) [Code]: downgrade the log level of error message from subprocess (elastic#42925) [Code] Cancel clone/update job in the middle if disk space over the watermark (elastic#42890) Add Kibana App specific URL to the help menu (elastic#34739) (elastic#42580) [Maps] refactor createShapeFilterWithMeta to support more than just polygons (elastic#43042) Skip flaky es_ui_shared/request tests. Pass uiSettings to all data plugin services (elastic#42159) [SIEM] Upgrades react-redux and utilize React.memo for performance gains (elastic#43029) [skip-ci][Maps] add maki icon sheet to docs (elastic#43063) Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (elastic#41305) Update dependency commander to v3 (elastic#43041) Update dependency @percy/agent to ^0.10.0 (elastic#40517) [Maps] only show top hits checkbox if index has date fields (elastic#43056) run chained_controls on Firefox to catch regression (elastic#43044) fixing issue with dashboard csv download (elastic#42964) Expose task manager as plugin instead of server argument (elastic#42966) Expose createRouter from HttpService, prepare handlers for context introduction (elastic#42686) [Code] disk watermark supports percentage and absolute modes (elastic#42987) [apps/dashboard] skip part of filtering tests on FF (elastic#43047) [ML] Kibana management jobs list (elastic#42570) [ML] Fix check for watcher being enabled (elastic#43025) ...

elasticmachine · 2019-08-12T10:38:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Looks great! I checked out the UX locally. You might want to get someone else to review the code because I didn't look very closely.

Originally I was thinking of something like this:

But I think the location you chose is better. However, there's no label which isn't so great for screen-readers. The space gets a bit cramped, too. I think you might want to reimplement your dropdown solution in the location where your checkboxes are, using a button that says "Select metrics" which would address the accessibility issue. Feel free to check in with the design team and get some guidance from them. I don't consider this a blocker though.

cjcenizal · 2019-08-12T23:51:18Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+        await addFieldToList('date');
+      });
+
+      const getFieldListTableRows = () => {


Would it make sense to move these helpers into helpers/job_create.helpers.js? That would deflate this file a bit and make them more reusable.

cjcenizal · 2019-08-12T23:51:59Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+        let fieldChooserColumn = getFieldChooserColumnForRow(0);
+        expectAllFieldChooserInputs(fieldChooserColumn, true);
+
+


Nit: extra newline

cjcenizal · 2019-08-12T23:55:48Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+
+        rows.forEach((row, idx) => {
+          const [, metricTypeCol ] = row.columns;
+          if (metricTypeCol.value !== 'numeric') {


Could you use filter here instead?

rows.filter((row) => { const [, metricTypeCol ] = row.columns; return metricTypeCol.value === 'numeric'; }).forEach((row, idx) => { const fieldChooser = getFieldChooserColumnForRow(idx); fieldChooser.reactWrapper.find('input').forEach(input => { const props = input.props(); if (props['data-test-subj'].endsWith('avg')) { expect(props.checked).toBe(true); } else { expect(props.checked).toBe(false); } }); });

cjcenizal · 2019-08-13T00:21:10Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+            <EuiFlexGroup justifyContent={'flexStart'} alignItems={'center'}>
+              <EuiFlexItem grow={false}>
+                <FieldChooser
+                  key={'stepMetricsFieldChooser'}


Minor nit: this can be a string "stepMetricsFieldChooser" without the enclosing braces. This comment applies to a few other places too.

- Refactor test helpers for metric step - Cleanup some JS style nits - Make popover button disabled if all metric checkboxes are disabled

elasticmachine · 2019-08-13T10:24:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jloleysens · 2019-08-13T10:26:38Z

Retest

elasticmachine · 2019-08-13T11:36:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…p-metrics-selectall * 'master' of github.com:elastic/kibana: (27 commits) [ML] Data Frames: Analytics job creation. (elastic#43102) [Vis Default editor] Fix issue with Rollup (elastic#42430) [Vis: Default editor] EUIficate Markdown tab (elastic#42677) [New Platform Migration Phase I]: update dateHisogramInterval & parseEsInterval imports (elastic#42917) [Infra UI] Add AWS metrics to node detail page (elastic#42153) update apm index pattern (elastic#43106) [SIEM] Toggle Column / Code Coverage and Cypress (elastic#42766) skip failing test (elastic#43163) [code] Add option to turn the go dependency download on/off. (elastic#43096) disable visual regression jobs Removed dead code (elastic#42774) fixes csv export of saved searches that have _source field (elastic#43123) Export missing Context types (elastic#43051) Update dependency supports-color to v7 (elastic#43064) switch to icon type string instead of node (elastic#43111) [Maps] Enable borders for icon symbols (elastic#43066) [ftr] enable visualRegression jobs (elastic#42989) [ML] Converting single to multi metric job (elastic#42532) fix(NA): dont clean dll module if it is a package json file (elastic#42904) [Logs UI] Add link from the sample web logs to the Logs UI (elastic#42635) ...

elasticmachine · 2019-08-13T12:41:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

@jloleysens nice work! Tested locally and works great. Left a couple comments in the code.

alisonelizabeth · 2019-08-14T13:18:49Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+      it('should deselect all of the metric types across rows (column-wise)',  () => {
+        const selectAllAvgCheckbox = find('rollupJobMetricsSelectAllCheckbox-avg');
+        selectAllAvgCheckbox.last().simulate('change', { checked: true });
+        selectAllAvgCheckbox.last().simulate('change', { checked: false });


can you explain why both of these lines are needed?

alisonelizabeth · 2019-08-14T13:18:54Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+
+      it('should correctly select across rows and columns',  async () => {
+        /**
+         * Tricky test case where we want to determine that the column-wise and row-wise


thanks for adding these comments!

alisonelizabeth · 2019-08-14T13:18:59Z

x-pack/legacy/plugins/rollup/__jest__/client_integration/job_create_metrics.test.js

+        // Select All
+        selectAllCheckbox.simulate('change', { checked: true });
+        // Deselect All
+        selectAllCheckbox.simulate('change', { checked: false });


similar question as above - are these two steps needed? by default, will the checkboxes not be unselected?

In this case we want to see that selecting some checkboxes column wise, then selecting the row-wise and then de-selecting them row-wise doesn't leave us in a weird state where some checkboxes (esp. All) are still selected for that row. Will add comment though :).

alisonelizabeth · 2019-08-14T13:21:12Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+const metricTypesConfig = (function () {
+  return [
+    {
+      type: 'avg',


i wonder if these types should belong in a constants file - that way they could be reused in the tests too. WDYT?

Sounds like a plan! Not exactly sure where I'd use them in my spec - will take a look-see.

i thought i saw use of avg, max, etc., in the tests, so was just wondering if it would be useful to extract.

alisonelizabeth · 2019-08-14T13:25:44Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+  static chooserColumns = [
+    {
+      field: 'name',
+      name: 'Field',


alisonelizabeth · 2019-08-14T13:25:51Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+    },
+    {
+      field: 'type',
+      name: 'Type',


alisonelizabeth · 2019-08-14T13:32:31Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+
+    /**
+     * Look at all the metric configs and include the special "All" checkbox which adds the ability
+     * to select a all the checkboxes across columns and rows.


typo: select a all --> select all

alisonelizabeth · 2019-08-14T13:35:05Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+          id={`${idx}-select-all-checkbox`}
+          data-test-subj={`rollupJobMetricsSelectAllCheckbox-${metricType}`}
+          disabled={isDisabled}
+          label={<span style={{ whiteSpace: 'nowrap' }}>{label}</span>}


can we add this to a .scss file rather than having an inline style here?

alisonelizabeth · 2019-08-14T13:36:51Z

x-pack/legacy/plugins/rollup/public/crud_app/sections/job_create/steps/step_metrics.js

+
+  getMetricsSelectAllMenu() {
+    const {
+      jsxElements: checkboxElements,


would it make sense to rename jsxElements to checkboxElements in renderMetricsSelectAllCheckboxes()?

- Removed unnecessary styling - Fixed typos - Updated comments - i18n

elasticmachine · 2019-08-14T19:29:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…p-metrics-selectall * 'master' of github.com:elastic/kibana: (50 commits) [Uptime] update monitor list configs for mobile view (elastic#43218) [APM] Local UI filters (elastic#41588) [Code] Upgrade ctags langserver (elastic#43252) [Code] show multiple definition results in panel (elastic#43249) Adds Metric Type to full screen launch tracking (elastic#42692) [Canvas] Convert Autocomplete to Typescript (elastic#42502) [telemetry] add spacesEnabled config back to xpack_main (elastic#43312) [ML] Adds DF Transform Analytics list to Kibana management (elastic#43151) Add TLS client authentication support. (elastic#43090) [csp] Telemetry for csp configuration (elastic#43223) [SIEM] Run Cypress Tests Against Elastic Cloud & Cypress Command Line / Reporting (elastic#42804) docs: add tip on agent config in a dt (elastic#43301) [ML] Adding bucket span estimator to new wizards (elastic#43288) disable flaky tests (elastic#43017) Fix percy target branch for PRs (elastic#43160) [ML] Adding post create job options (elastic#43205) Restore discover histogram selection triggering fetch (elastic#43097) Per panel time range (elastic#43153) [Infra UI] Add APM to Metadata Endpoint (elastic#42197) Sentence case copy changes (elastic#43215) ...

elasticmachine · 2019-08-15T14:34:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alisonelizabeth

LGTM - thanks for making the changes @jloleysens!

* remove duplicate columns declaration * First iteration of select all menu working * A lot of bugfixes and started on row-level selection using flatMap * Wip on row selection * Fix formatting * More bug fixes and and first iteration of row selection and column selection working * Minor refactors, cleaned up logging. Still has nasty UX bug on Popover menu * Added translations and empty tests * Tests are WiP * Move checkboxes out of popover * Implemented first iteration of select all functionality * Move row select all into checkbox area * Remove unused code and simplify select all if-else * First iteration of tests * fix tests and update name data-test-subj name * Move checkboxes into popover with button * - Fix tests - Refactor test helpers for metric step - Cleanup some JS style nits - Make popover button disabled if all metric checkboxes are disabled * Remove extra newline * - Moved constants from metrics to own file so that it's more re-usable - Removed unnecessary styling - Fixed typos - Updated comments - i18n

jloleysens added 7 commits August 6, 2019 15:32

remove duplicate columns declaration

b643f7e

First iteration of select all menu working

d3e08f3

A lot of bugfixes and started on row-level selection using flatMap

2e24223

Wip on row selection

428a17a

Fix formatting

d6eda9e

More bug fixes and and first iteration of row selection and column se…

82dd844

…lection working

Minor refactors, cleaned up logging. Still has nasty UX bug on Popove…

d01303e

…r menu

jloleysens added release_note:enhancement enhancement New value added to drive a business result v8.0.0 Feature:Rollups v7.4.0 labels Aug 8, 2019

jloleysens self-assigned this Aug 8, 2019

Added translations and empty tests

6d7e3f2

Tests are WiP

58e5bf8

jloleysens added 5 commits August 9, 2019 13:06

Move checkboxes out of popover

50f531d

Implemented first iteration of select all functionality

08ed087

Move row select all into checkbox area

674048b

Remove unused code and simplify select all if-else

74458f8

First iteration of tests

0427cd9

jloleysens requested a review from cjcenizal August 9, 2019 13:45

jloleysens marked this pull request as ready for review August 9, 2019 13:45

jloleysens added 2 commits August 9, 2019 16:52

fix tests and update name data-test-subj name

120de8e

jloleysens added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Aug 12, 2019

cjcenizal approved these changes Aug 13, 2019

View reviewed changes

jloleysens requested a review from sebelga August 13, 2019 08:02

jloleysens added 3 commits August 13, 2019 10:51

Move checkboxes into popover with button

139559f

- Fix tests

ba67cf4

- Refactor test helpers for metric step - Cleanup some JS style nits - Make popover button disabled if all metric checkboxes are disabled

Remove extra newline

a2a8661

jloleysens requested review from alisonelizabeth and removed request for sebelga August 14, 2019 09:39

alisonelizabeth reviewed Aug 14, 2019

View reviewed changes

- Moved constants from metrics to own file so that it's more re-usable

0505101

- Removed unnecessary styling - Fixed typos - Updated comments - i18n

alisonelizabeth approved these changes Aug 15, 2019

View reviewed changes

jloleysens merged commit bd8984b into elastic:master Aug 15, 2019

jloleysens deleted the feature/rollup-metrics-selectall branch August 15, 2019 14:46

jloleysens mentioned this pull request Aug 15, 2019

[7.x] [Rollup] Metrics multiple selection (#42927) #43353

Merged

cjcenizal mentioned this pull request Aug 30, 2019

Select all functionality for Rollup Job Wizard metrics #41902

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Rollup] Metrics multiple selection #42927

[Rollup] Metrics multiple selection #42927

jloleysens commented Aug 8, 2019 •

edited

Loading

elasticmachine commented Aug 8, 2019

elasticmachine commented Aug 8, 2019

elasticmachine commented Aug 9, 2019

elasticmachine commented Aug 9, 2019

elasticmachine commented Aug 12, 2019

cjcenizal left a comment

cjcenizal Aug 12, 2019

cjcenizal Aug 12, 2019

cjcenizal Aug 12, 2019

cjcenizal Aug 13, 2019

elasticmachine commented Aug 13, 2019

jloleysens commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

alisonelizabeth left a comment

alisonelizabeth Aug 14, 2019

jloleysens Aug 14, 2019

alisonelizabeth Aug 14, 2019

alisonelizabeth Aug 14, 2019

jloleysens Aug 14, 2019

alisonelizabeth Aug 14, 2019

jloleysens Aug 14, 2019

alisonelizabeth Aug 15, 2019

alisonelizabeth Aug 14, 2019

alisonelizabeth Aug 14, 2019

alisonelizabeth Aug 14, 2019

alisonelizabeth Aug 14, 2019

alisonelizabeth Aug 14, 2019

elasticmachine commented Aug 14, 2019

elasticmachine commented Aug 15, 2019

alisonelizabeth left a comment

		let fieldChooserColumn = getFieldChooserColumnForRow(0);
		expectAllFieldChooserInputs(fieldChooserColumn, true);

[Rollup] Metrics multiple selection #42927

[Rollup] Metrics multiple selection #42927

Conversation

jloleysens commented Aug 8, 2019 • edited Loading

Summary

Screenshots

Checklist

For maintainers

elasticmachine commented Aug 8, 2019

💔 Build Failed

elasticmachine commented Aug 8, 2019

💚 Build Succeeded

elasticmachine commented Aug 9, 2019

💔 Build Failed

elasticmachine commented Aug 9, 2019

💚 Build Succeeded

elasticmachine commented Aug 12, 2019

💚 Build Succeeded

cjcenizal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 13, 2019

💔 Build Failed

jloleysens commented Aug 13, 2019

elasticmachine commented Aug 13, 2019

💔 Build Failed

elasticmachine commented Aug 13, 2019

💚 Build Succeeded

alisonelizabeth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 14, 2019

💚 Build Succeeded

elasticmachine commented Aug 15, 2019

💚 Build Succeeded

alisonelizabeth left a comment

Choose a reason for hiding this comment

jloleysens commented Aug 8, 2019 •

edited

Loading