chore: Select component refactoring - SelectControl - Iteration 5 (#16510)

* Refactor Select DatasourceEditor * Fire onChange with allowNewOptions * Clean up * Refactor Select in AnnotationLayer * Handle on clear * Update tests * Refactor Select in SpatialControl * Show search * Refactor Select in FilterBox * Remove search where unnecessary * Update SelectControl - WIP * Refactor Controls * Update SelectControl tests * Clean up * Test allowNewOptions false * Use SelectControl AnnotationLayer * Use SelectControl SpatialControl * Clean up * Render custom label * Show search * Implement filterOption * Improve filterOption * Update Cypress * Update Cypress table test * Use value for defaultValue * Merge with latest changes * Reconcile with latest Select changes * Update superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Update superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com> * Revert changes to test * Call onPopoverClear when v value is undefined Co-authored-by: Michael S. Molina <70410625+michael-s-molina@users.noreply.github.com>
2026-06-01 05:39:17 +00:00 · 2021-10-04 18:24:41 +03:00
parent 1ab36c94f3
commit 3f0756f637
14 changed files with 233 additions and 412 deletions
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx
@@ -70,8 +70,6 @@ const propTypes = {
  addAnnotationLayer: PropTypes.func,
  removeAnnotationLayer: PropTypes.func,
  close: PropTypes.func,
-
-  onPopoverClear: PropTypes.func,
 };

 const defaultProps = {
@@ -95,7 +93,6 @@ const defaultProps = {
  addAnnotationLayer: () => {},
  removeAnnotationLayer: () => {},
  close: () => {},
-  onPopoverClear: () => {},
 };

 export default class AnnotationLayer extends React.PureComponent {
@@ -172,7 +169,6 @@ export default class AnnotationLayer extends React.PureComponent {
    );
    this.handleValue = this.handleValue.bind(this);
    this.isValidForm = this.isValidForm.bind(this);
-    this.popoverClearWrapper = this.popoverClearWrapper.bind(this);
  }

  componentDidMount() {
@@ -238,15 +234,6 @@ export default class AnnotationLayer extends React.PureComponent {
    return !errors.filter(x => x).length;
  }

-  popoverClearWrapper(value, actionMeta, callback) {
-    if (callback) {
-      callback(value);
-    }
-    if (actionMeta?.action === 'clear') {
-      this.props.onPopoverClear(true);
-    }
-  }
-
  handleAnnotationType(annotationType) {
    this.setState({
      annotationType,
@@ -266,7 +253,7 @@ export default class AnnotationLayer extends React.PureComponent {
  handleValue(value) {
    this.setState({
      value,
-      descriptionColumns: null,
+      descriptionColumns: [],
      intervalEndColumn: null,
      timeColumn: null,
      titleColumn: null,
@@ -409,6 +396,7 @@ export default class AnnotationLayer extends React.PureComponent {
    if (requiresQuery(sourceType)) {
      return (
        <SelectControl
+          ariaLabel={t('Annotation layer value')}
          name="annotation-layer-value"
          showHeader
          hovered
@@ -418,9 +406,7 @@ export default class AnnotationLayer extends React.PureComponent {
          options={valueOptions}
          isLoading={isLoadingOptions}
          value={value}
-          onChange={(value, _, actionMeta) =>
-            this.popoverClearWrapper(value, actionMeta, this.handleValue)
-          }
+          onChange={this.handleValue}
          validationErrors={!value ? ['Mandatory'] : []}
          optionRenderer={this.renderOption}
        />
@@ -479,14 +465,17 @@ export default class AnnotationLayer extends React.PureComponent {
            {(annotationType === ANNOTATION_TYPES.EVENT ||
              annotationType === ANNOTATION_TYPES.INTERVAL) && (
              <SelectControl
+                ariaLabel={t('Annotation layer time column')}
                hovered
                name="annotation-layer-time-column"
                label={
                  annotationType === ANNOTATION_TYPES.INTERVAL
-                    ? 'Interval start column'
-                    : 'Event time column'
+                    ? t('Interval start column')
+                    : t('Event time column')
                }
-                description="This column must contain date/time information."
+                description={t(
+                  'This column must contain date/time information.',
+                )}
                validationErrors={!timeColumn ? ['Mandatory'] : []}
                clearable={false}
                options={timeColumnOptions}
@@ -496,48 +485,42 @@ export default class AnnotationLayer extends React.PureComponent {
            )}
            {annotationType === ANNOTATION_TYPES.INTERVAL && (
              <SelectControl
+                ariaLabel={t('Annotation layer interval end')}
                hovered
                name="annotation-layer-intervalEnd"
-                label="Interval End column"
-                description="This column must contain date/time information."
+                label={t('Interval End column')}
+                description={t(
+                  'This column must contain date/time information.',
+                )}
                validationErrors={!intervalEndColumn ? ['Mandatory'] : []}
                options={columns}
                value={intervalEndColumn}
-                onChange={(value, _, actionMeta) =>
-                  this.popoverClearWrapper(value, actionMeta, v =>
-                    this.setState({ intervalEndColumn: v }),
-                  )
-                }
+                onChange={value => this.setState({ intervalEndColumn: value })}
              />
            )}
            <SelectControl
+              ariaLabel={t('Annotation layer title column')}
              hovered
              name="annotation-layer-title"
-              label="Title Column"
-              description="Pick a title for you annotation."
+              label={t('Title Column')}
+              description={t('Pick a title for you annotation.')}
              options={[{ value: '', label: 'None' }].concat(columns)}
              value={titleColumn}
-              onChange={(value, _, actionMeta) =>
-                this.popoverClearWrapper(value, actionMeta, v =>
-                  this.setState({ titleColumn: v }),
-                )
-              }
+              onChange={value => this.setState({ titleColumn: value })}
            />
            {annotationType !== ANNOTATION_TYPES.TIME_SERIES && (
              <SelectControl
+                ariaLabel={t('Annotation layer description columns')}
                hovered
                name="annotation-layer-title"
-                label="Description Columns"
-                description={`Pick one or more columns that should be shown in the
-                  annotation. If you don't select a column all of them will be shown.`}
+                label={t('Description Columns')}
+                description={t(
+                  "Pick one or more columns that should be shown in the annotation. If you don't select a column all of them will be shown.",
+                )}
                multi
                options={columns}
                value={descriptionColumns}
-                onChange={(value, _, actionMeta) =>
-                  this.popoverClearWrapper(value, actionMeta, v =>
-                    this.setState({ descriptionColumns: v }),
-                  )
-                }
+                onChange={value => this.setState({ descriptionColumns: value })}
              />
            )}
            <div style={{ marginTop: '1rem' }}>
@@ -629,6 +612,7 @@ export default class AnnotationLayer extends React.PureComponent {
        info={t('Configure your how you overlay is displayed here.')}
      >
        <SelectControl
+          ariaLabel={t('Annotation layer stroke')}
          name="annotation-layer-stroke"
          label={t('Style')}
          // see '../../../visualizations/nvd3_vis.css'
@@ -643,6 +627,7 @@ export default class AnnotationLayer extends React.PureComponent {
          onChange={v => this.setState({ style: v })}
        />
        <SelectControl
+          ariaLabel={t('Annotation layer opacity')}
          name="annotation-layer-opacity"
          label={t('Opacity')}
          // see '../../../visualizations/nvd3_vis.css'
@@ -653,11 +638,7 @@ export default class AnnotationLayer extends React.PureComponent {
            { value: 'opacityHigh', label: '0.8' },
          ]}
          value={opacity}
-          onChange={(value, _, actionMeta) =>
-            this.popoverClearWrapper(value, actionMeta, v =>
-              this.setState({ opacity: v }),
-            )
-          }
+          onChange={value => this.setState({ opacity: value })}
        />
        <div>
          <ControlHeader label={t('Color')} />
@@ -746,6 +727,7 @@ export default class AnnotationLayer extends React.PureComponent {
                onChange={v => this.setState({ show: !v })}
              />
              <SelectControl
+                ariaLabel={t('Annotation layer type')}
                hovered
                description={t('Choose the annotation layer type')}
                label={t('Annotation layer type')}
@@ -757,19 +739,14 @@ export default class AnnotationLayer extends React.PureComponent {
              />
              {supportedSourceTypes.length > 0 && (
                <SelectControl
+                  ariaLabel={t('Annotation source type')}
                  hovered
-                  description="Choose the source of your annotations"
-                  label="Annotation Source"
+                  description={t('Choose the source of your annotations')}
+                  label={t('Annotation Source')}
                  name="annotation-source-type"
                  options={supportedSourceTypes}
                  value={sourceType}
-                  onChange={(value, _, actionMeta) =>
-                    this.popoverClearWrapper(
-                      value,
-                      actionMeta,
-                      this.handleAnnotationSourceType,
-                    )
-                  }
+                  onChange={this.handleAnnotationSourceType}
                  validationErrors={!sourceType ? [t('Mandatory')] : []}
                />
              )}
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.test.tsx
@@ -84,10 +84,22 @@ test('renders extra checkboxes when type is time series', async () => {
 });

 test('enables apply and ok buttons', async () => {
-  await waitForRender();
-  userEvent.type(screen.getByLabelText('Name'), 'Test');
-  userEvent.type(screen.getByLabelText('Formula'), '2x');
-  await waitFor(() => {
+  const { container } = render(<AnnotationLayer {...defaultProps} />);
+
+  waitFor(() => {
+    expect(container).toBeInTheDocument();
+  });
+
+  const nameInput = screen.getByRole('textbox', { name: 'Name' });
+  const formulaInput = screen.getByRole('textbox', { name: 'Formula' });
+
+  expect(nameInput).toBeInTheDocument();
+  expect(formulaInput).toBeInTheDocument();
+
+  userEvent.type(nameInput, 'Name');
+  userEvent.type(formulaInput, '2x');
+
+  waitFor(() => {
    expect(screen.getByRole('button', { name: 'Apply' })).toBeEnabled();
    expect(screen.getByRole('button', { name: 'OK' })).toBeEnabled();
  });
@@ -134,12 +146,17 @@ test('renders chart options', async () => {
  await waitForRender({
    annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
  });
-  userEvent.click(screen.getByText('2 option(s)'));
-  userEvent.click(screen.getByText('Superset annotation'));
-  expect(await screen.findByLabelText('Annotation layer')).toBeInTheDocument();
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation source type' }),
+  );
  userEvent.click(screen.getByText('Superset annotation'));
+  expect(screen.getByText('Annotation layer')).toBeInTheDocument();
+
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation source type' }),
+  );
  userEvent.click(screen.getByText('Table'));
-  expect(await screen.findByLabelText('Chart')).toBeInTheDocument();
+  expect(screen.getByText('Chart')).toBeInTheDocument();
 });

 test('keeps apply disabled when missing required fields', async () => {
@@ -147,18 +164,28 @@ test('keeps apply disabled when missing required fields', async () => {
    annotationType: ANNOTATION_TYPES_METADATA.EVENT.value,
    sourceType: 'Table',
  });
-  userEvent.click(screen.getByText('1 option(s)'));
-  await waitFor(() => userEvent.click(screen.getByText('Chart A')));
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation layer value' }),
+  );
+  userEvent.click(await screen.findByText('Chart A'));
  expect(
    screen.getByText('Annotation Slice Configuration'),
  ).toBeInTheDocument();

  userEvent.click(screen.getByRole('button', { name: 'Automatic Color' }));
-  userEvent.click(screen.getByLabelText('Title Column'));
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation layer title column' }),
+  );
  userEvent.click(screen.getByText('None'));
-  userEvent.click(screen.getByLabelText('Style'));
+  userEvent.click(screen.getByText('Style'));
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation layer stroke' }),
+  );
  userEvent.click(screen.getByText('Dashed'));
-  userEvent.click(screen.getByLabelText('Opacity'));
+  userEvent.click(screen.getByText('Opacity'));
+  userEvent.click(
+    screen.getByRole('combobox', { name: 'Annotation layer opacity' }),
+  );
  userEvent.click(screen.getByText('0.5'));

  const checkboxes = screen.getAllByRole('checkbox');
--- a/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.jsx
+++ b/superset-frontend/src/explore/components/controls/AnnotationLayerControl/index.jsx
@@ -62,12 +62,10 @@ class AnnotationLayerControl extends React.PureComponent {
    this.state = {
      popoverVisible: {},
      addedAnnotationIndex: null,
-      popoverClear: false,
    };
    this.addAnnotationLayer = this.addAnnotationLayer.bind(this);
    this.removeAnnotationLayer = this.removeAnnotationLayer.bind(this);
    this.handleVisibleChange = this.handleVisibleChange.bind(this);
-    this.handlePopoverClear = this.handlePopoverClear.bind(this);
  }

  componentDidMount() {
@@ -105,19 +103,9 @@ class AnnotationLayerControl extends React.PureComponent {
  }

  handleVisibleChange(visible, popoverKey) {
-    if (!this.state.popoverClear) {
-      this.setState(prevState => ({
-        popoverVisible: { ...prevState.popoverVisible, [popoverKey]: visible },
-      }));
-    } else {
-      this.handlePopoverClear(false);
-    }
-  }
-
-  handlePopoverClear(popoverClear) {
-    this.setState({
-      popoverClear,
-    });
+    this.setState(prevState => ({
+      popoverVisible: { ...prevState.popoverVisible, [popoverKey]: visible },
+    }));
  }

  removeAnnotationLayer(annotation) {
@@ -143,7 +131,6 @@ class AnnotationLayerControl extends React.PureComponent {
            this.handleVisibleChange(false, popoverKey);
            this.setState({ addedAnnotationIndex: null });
          }}
-          onPopoverClear={this.handlePopoverClear}
        />
      </div>
    );