From 913df18075a75e781cb96989320e7e0d5c399146 Mon Sep 17 00:00:00 2001
From: Geido <60598000+geido@users.noreply.github.com>
Date: Mon, 27 Sep 2021 17:25:51 +0300
Subject: [PATCH] chore: Select component refactoring - MetricControl -
Iteration 5 (#16423)
* Refactor Select
* Render saved metric option
* Update tests
* Fix Cypress and ariaLabels
* Fix lint
* Lint Cypress
* Reconcile with latest Select changes
---
.../integration/explore/AdhocMetrics.test.ts | 8 +-
.../explore/visualizations/line.test.ts | 6 +-
.../AdhocMetricEditPopover_spec.jsx | 6 +-
.../AdhocMetricEditPopover/index.jsx | 90 ++++++++-----------
4 files changed, 52 insertions(+), 58 deletions(-)
diff --git a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
index d8d06862e4..9983e72996 100644
--- a/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/explore/AdhocMetrics.test.ts
@@ -42,8 +42,12 @@ describe('AdhocMetrics', () => {
cy.get('[data-test="AdhocMetricEditTitle#trigger"]').click();
cy.get('[data-test="AdhocMetricEditTitle#input"]').type(metricName);
- cy.get('[name="select-column"]').click().type('num_girls{enter}');
- cy.get('[name="select-aggregate"]').click().type('sum{enter}');
+ cy.get('input[aria-label="Select column"]')
+ .click()
+ .type('num_girls{enter}');
+ cy.get('input[aria-label="Select aggregate options"]')
+ .click()
+ .type('sum{enter}');
cy.get('[data-test="AdhocMetricEdit#save"]').contains('Save').click();
diff --git a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
index 56a37ce121..676c3b6d53 100644
--- a/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
+++ b/superset-frontend/cypress-base/cypress/integration/explore/visualizations/line.test.ts
@@ -45,8 +45,10 @@ describe('Visualization > Line', () => {
// Title edit for saved metrics is disabled - switch to Simple
cy.get('[id="adhoc-metric-edit-tabs-tab-SIMPLE"]').click();
- cy.get('[name="select-column"]').click().type('num{enter}');
- cy.get('[name="select-aggregate"]').click().type('sum{enter}');
+ cy.get('input[aria-label="Select column"]').click().type('num{enter}');
+ cy.get('input[aria-label="Select aggregate options"]')
+ .click()
+ .type('sum{enter}');
cy.get('[data-test="AdhocMetricEdit#save"]').contains('Save').click();
cy.get('.text-danger').should('not.exist');
diff --git a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
index 3e436dbe2d..507b0c95f7 100644
--- a/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
+++ b/superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
@@ -73,7 +73,7 @@ describe('AdhocMetricEditPopover', () => {
it('overwrites the adhocMetric in state with onColumnChange', () => {
const { wrapper } = setup();
- wrapper.instance().onColumnChange(columns[0].id);
+ wrapper.instance().onColumnChange(columns[0].column_name);
expect(wrapper.state('adhocMetric')).toEqual(
sumValueAdhocMetric.duplicateWith({ column: columns[0] }),
);
@@ -100,7 +100,7 @@ describe('AdhocMetricEditPopover', () => {
expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
wrapper.instance().onColumnChange(null);
expect(wrapper.find(Button).find({ disabled: true })).toExist();
- wrapper.instance().onColumnChange(columns[0].id);
+ wrapper.instance().onColumnChange(columns[0].column_name);
expect(wrapper.find(Button).find({ disabled: true })).not.toExist();
wrapper.instance().onAggregateChange(null);
expect(wrapper.find(Button).find({ disabled: true })).toExist();
@@ -109,7 +109,7 @@ describe('AdhocMetricEditPopover', () => {
it('highlights save if changes are present', () => {
const { wrapper } = setup();
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).not.toExist();
- wrapper.instance().onColumnChange(columns[1].id);
+ wrapper.instance().onColumnChange(columns[1].column_name);
expect(wrapper.find(Button).find({ buttonStyle: 'primary' })).toExist();
});
diff --git a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
index 3ad0d92de6..59760e9766 100644
--- a/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
+++ b/superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
@@ -21,7 +21,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import Tabs from 'src/components/Tabs';
import Button from 'src/components/Button';
-import { NativeSelect as Select } from 'src/components/Select';
+import { Select } from 'src/components';
import { t, styled } from '@superset-ui/core';
import { Form, FormItem } from 'src/components/Form';
@@ -162,8 +162,10 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
);
}
- onColumnChange(columnId) {
- const column = this.props.columns.find(column => column.id === columnId);
+ onColumnChange(columnName) {
+ const column = this.props.columns.find(
+ column => column.column_name === columnName,
+ );
this.setState(prevState => ({
adhocMetric: prevState.adhocMetric.duplicateWith({
column,
@@ -184,9 +186,9 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
}));
}
- onSavedMetricChange(savedMetricId) {
+ onSavedMetricChange(savedMetricName) {
const savedMetric = this.props.savedMetricsOptions.find(
- metric => metric.id === savedMetricId,
+ metric => metric.metric_name === savedMetricName,
);
this.setState(prevState => ({
savedMetric,
@@ -262,6 +264,10 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
return ;
}
+ renderMetricOption(savedMetric) {
+ return ;
+ }
+
render() {
const {
adhocMetric: propsAdhocMetric,
@@ -290,34 +296,30 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
// autofocus on column if there's no value in column; otherwise autofocus on aggregate
const columnSelectProps = {
+ ariaLabel: t('Select column'),
placeholder: t('%s column(s)', columns.length),
value: columnValue,
onChange: this.onColumnChange,
allowClear: true,
- showSearch: true,
autoFocus: !columnValue,
- filterOption: (input, option) =>
- option.filterBy.toLowerCase().indexOf(input.toLowerCase()) >= 0,
};
const aggregateSelectProps = {
+ ariaLabel: t('Select aggregate options'),
placeholder: t('%s aggregates(s)', AGGREGATES_OPTIONS.length),
value: adhocMetric.aggregate || adhocMetric.inferSqlExpressionAggregate(),
onChange: this.onAggregateChange,
allowClear: true,
autoFocus: !!columnValue,
- showSearch: true,
};
const savedSelectProps = {
+ ariaLabel: t('Select saved metrics'),
placeholder: t('%s saved metric(s)', savedMetricsOptions?.length ?? 0),
- value: savedMetric?.verbose_name || savedMetric?.metric_name,
+ value: savedMetric?.metric_name,
onChange: this.onSavedMetricChange,
allowClear: true,
- showSearch: true,
autoFocus: true,
- filterOption: (input, option) =>
- option.filterBy.toLowerCase().indexOf(input.toLowerCase()) >= 0,
};
if (this.props.datasourceType === 'druid' && aggregateSelectProps.options) {
@@ -354,55 +356,41 @@ export default class AdhocMetricEditPopover extends React.PureComponent {
({
+ value: savedMetric.metric_name,
+ label: savedMetric.metric_name,
+ customLabel: this.renderMetricOption(savedMetric),
+ key: savedMetric.id,
+ }))
+ : []
+ }
{...savedSelectProps}
- name="select-saved"
- getPopupContainer={triggerNode => triggerNode.parentNode}
- >
- {Array.isArray(savedMetricsOptions) &&
- savedMetricsOptions.map(savedMetric => (
-
-
-
- ))}
-
+ />
+ />
+ />