From 819118be13147129a42230bbeb893c6cf7e387f1 Mon Sep 17 00:00:00 2001
From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com>
Date: Fri, 25 Jun 2021 13:33:12 -0300
Subject: [PATCH] fix: Enlarged select filter value (#15373)

* fix: Enlarged select filter value

* fix: Makes the label take the whole width

* fix: Moves the required icon before the cascade icon

* fix: Fixes the cascading icon overlap with big texts
---
 .../FilterControls/FilterControl.tsx          |  58 +++++++---
 .../FilterBar/FilterControls/FilterValue.tsx  |  31 +++---
 .../FiltersConfigForm/DefaultValue.tsx        |   6 --
 .../FiltersConfigForm/FiltersConfigForm.tsx   |  18 +++-
 .../FiltersConfigForm/state.ts                |   3 +-
 .../components/Select/SelectFilterPlugin.tsx  | 101 ++++++++----------
 .../src/filters/components/common.ts          |   2 +-
 7 files changed, 116 insertions(+), 103 deletions(-)

diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
index 480bde31a3..db57fdf11b 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx
@@ -18,11 +18,24 @@
  */
 import React from 'react';
 import { styled } from '@superset-ui/core';
+import { Form, FormItem } from 'src/components/Form';
 import FilterValue from './FilterValue';
 import { FilterProps } from './types';
+import { checkIsMissingRequiredValue } from '../utils';
+
+const StyledFormItem = styled(FormItem)`
+  & label {
+    width: 100%;
+    padding-right: ${({ theme }) => theme.gridUnit * 11}px;
+  }
+`;
+
+const StyledIcon = styled.div`
+  position: absolute;
+  right: 0;
+`;
 
 const StyledFilterControlTitle = styled.h4`
-  width: 100%;
   font-size: ${({ theme }) => theme.typography.sizes.s}px;
   color: ${({ theme }) => theme.colors.grayscale.dark1};
   margin: 0;
@@ -37,7 +50,7 @@ const StyledFilterControlTitleBox = styled.div`
   margin-bottom: ${({ theme }) => theme.gridUnit}px;
 `;
 
-const StyledFilterControlContainer = styled.div`
+const StyledFilterControlContainer = styled(Form)`
   width: 100%;
 `;
 
@@ -50,21 +63,34 @@ const FilterControl: React.FC<FilterProps> = ({
   inView,
 }) => {
   const { name = '<undefined>' } = filter;
+
+  const isMissingRequiredValue = checkIsMissingRequiredValue(
+    filter,
+    filter.dataMask?.filterState,
+  );
+
   return (
-    <StyledFilterControlContainer>
-      <StyledFilterControlTitleBox>
-        <StyledFilterControlTitle data-test="filter-control-name">
-          {name}
-        </StyledFilterControlTitle>
-        <div data-test="filter-icon">{icon}</div>
-      </StyledFilterControlTitleBox>
-      <FilterValue
-        dataMaskSelected={dataMaskSelected}
-        filter={filter}
-        directPathToChild={directPathToChild}
-        onFilterSelectionChange={onFilterSelectionChange}
-        inView={inView}
-      />
+    <StyledFilterControlContainer layout="vertical">
+      <StyledFormItem
+        label={
+          <StyledFilterControlTitleBox>
+            <StyledFilterControlTitle data-test="filter-control-name">
+              {name}
+            </StyledFilterControlTitle>
+            <StyledIcon data-test="filter-icon">{icon}</StyledIcon>
+          </StyledFilterControlTitleBox>
+        }
+        required={filter?.controlValues?.enableEmptyFilter}
+        validateStatus={isMissingRequiredValue ? 'error' : undefined}
+      >
+        <FilterValue
+          dataMaskSelected={dataMaskSelected}
+          filter={filter}
+          directPathToChild={directPathToChild}
+          onFilterSelectionChange={onFilterSelectionChange}
+          inView={inView}
+        />
+      </StyledFormItem>
     </StyledFilterControlContainer>
   );
 };
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
index 59ad936e55..e5f6727d4d 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
@@ -19,10 +19,10 @@
 import React, { useEffect, useRef, useState } from 'react';
 import {
   QueryFormData,
-  styled,
   SuperChart,
   DataMask,
   t,
+  styled,
   Behavior,
   ChartDataResponseResult,
   JsonObject,
@@ -43,13 +43,14 @@ import { ClientErrorObject } from 'src/utils/getClientErrorObject';
 import { FilterProps } from './types';
 import { getFormData } from '../../utils';
 import { useCascadingFilters } from './state';
-import { checkIsMissingRequiredValue } from '../utils';
 
-const FilterItem = styled.div`
-  min-height: ${({ theme }) => theme.gridUnit * 11}px;
-  padding-bottom: ${({ theme }) => theme.gridUnit * 3}px;
-  & > div > div {
-    height: auto;
+const HEIGHT = 32;
+
+// Overrides superset-ui height with min-height
+const StyledDiv = styled.div`
+  & > div {
+    height: auto !important;
+    min-height: ${HEIGHT}px;
   }
 `;
 
@@ -195,35 +196,27 @@ const FilterValue: React.FC<FilterProps> = ({
     );
   }
 
-  const isMissingRequiredValue = checkIsMissingRequiredValue(
-    filter,
-    filter.dataMask?.filterState,
-  );
-
   return (
-    <FilterItem data-test="form-item-value">
+    <StyledDiv data-test="form-item-value">
       {isLoading ? (
         <Loading position="inline-centered" />
       ) : (
         <SuperChart
-          height={50}
+          height={HEIGHT}
           width="100%"
           formData={formData}
           // For charts that don't have datasource we need workaround for empty placeholder
           queriesData={hasDataSource ? state : [{ data: [{}] }]}
           chartType={filterType}
           behaviors={[Behavior.NATIVE_FILTER]}
-          filterState={{
-            ...filter.dataMask?.filterState,
-            validateMessage: isMissingRequiredValue && t('Value is required'),
-          }}
+          filterState={{ ...filter.dataMask?.filterState }}
           ownState={filter.dataMask?.ownState}
           enableNoResults={metadata?.enableNoResults}
           isRefreshing={isRefreshing}
           hooks={{ setDataMask, setFocusedFilter, unsetFocusedFilter }}
         />
       )}
-    </FilterItem>
+    </StyledDiv>
   );
 };
 
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx
index 85d55807b2..0b22dc704b 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DefaultValue.tsx
@@ -22,7 +22,6 @@ import {
   SetDataMaskHook,
   SuperChart,
   AppSection,
-  t,
 } from '@superset-ui/core';
 import { FormInstance } from 'antd/lib/form';
 import Loading from 'src/components/Loading';
@@ -57,10 +56,6 @@ const DefaultValue: FC<DefaultValueProps> = ({
       setLoading(true);
     }
   }, [hasDataset, queriesData]);
-  const value = formFilter.defaultDataMask?.filterState.value;
-  const isMissingRequiredValue =
-    (value === null || value === undefined) &&
-    formFilter?.controlValues?.enableEmptyFilter;
   return loading ? (
     <Loading position="inline-centered" />
   ) : (
@@ -79,7 +74,6 @@ const DefaultValue: FC<DefaultValueProps> = ({
       enableNoResults={enableNoResults}
       filterState={{
         ...formFilter.defaultDataMask?.filterState,
-        validateMessage: isMissingRequiredValue && t('Value is required'),
       }}
     />
   );
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
index f38d737e45..5a48837b63 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -137,14 +137,24 @@ export const StyledRowFormItem = styled(FormItem)`
 `;
 
 export const StyledRowSubFormItem = styled(FormItem)`
+  min-width: 50%;
+
   & .ant-form-item-label {
     padding-bottom: 0;
   }
 
+  .ant-form-item {
+    margin-bottom: 0;
+  }
+
   .ant-form-item-control-input-content > div > div {
     height: auto;
   }
 
+  .ant-form-item-extra {
+    display: none;
+  }
+
   & .ant-form-item-control-input {
     height: auto;
   }
@@ -801,7 +811,9 @@ const FiltersConfigForm = (
                       if (hasValue) {
                         return Promise.resolve();
                       }
-                      return Promise.reject();
+                      return Promise.reject(
+                        new Error(t('Default value is required')),
+                      );
                     },
                   },
                 ]}
@@ -1010,7 +1022,7 @@ const FiltersConfigForm = (
                   onChange={checked => onSortChanged(checked || undefined)}
                   initialValue={hasSorting}
                 >
-                  <StyledFormItem
+                  <StyledRowFormItem
                     name={[
                       'filters',
                       filterId,
@@ -1038,7 +1050,7 @@ const FiltersConfigForm = (
                         onSortChanged(value)
                       }
                     />
-                  </StyledFormItem>
+                  </StyledRowFormItem>
                   {hasMetrics && (
                     <StyledRowSubFormItem
                       name={['filters', filterId, 'sortMetric']}
diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
index 6cb292d5e3..fa6058de85 100644
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
+++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/state.ts
@@ -54,8 +54,7 @@ export const useDefaultValue = (
   filterToEdit?: Filter,
 ) => {
   const [hasDefaultValue, setHasPartialDefaultValue] = useState(
-    !!filterToEdit?.defaultDataMask?.filterState?.value ||
-      formFilter?.controlValues?.enableEmptyFilter,
+    !!filterToEdit?.defaultDataMask?.filterState?.value,
   );
   const [isRequired, setisRequired] = useState(
     formFilter?.controlValues?.enableEmptyFilter,
diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
index 9cf1b2e192..7e4ffaaef9 100644
--- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
+++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx
@@ -26,11 +26,9 @@ import {
   GenericDataType,
   JsonObject,
   smartDateDetailedFormatter,
-  styled,
   t,
   tn,
 } from '@superset-ui/core';
-import { FormItem } from 'src/components/Form';
 import React, {
   RefObject,
   ReactElement,
@@ -82,10 +80,6 @@ function reducer(
   }
 }
 
-const Error = styled.div`
-  color: ${({ theme }) => theme.colors.error.base};
-`;
-
 export default function PluginFilterSelect(props: PluginFilterSelectProps) {
   const {
     coltypeMap,
@@ -279,57 +273,52 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {
 
   return (
     <Styles height={height} width={width}>
-      <FormItem
-        validateStatus={filterState.validateMessage && 'error'}
-        extra={<Error>{filterState.validateMessage}</Error>}
+      <StyledSelect
+        allowClear
+        // @ts-ignore
+        value={filterState.value || []}
+        disabled={isDisabled}
+        showSearch={showSearch}
+        mode={multiSelect ? 'multiple' : undefined}
+        placeholder={placeholderText}
+        onSearch={searchWrapper}
+        onSelect={clearSuggestionSearch}
+        onBlur={handleBlur}
+        onDropdownVisibleChange={setIsDropdownVisible}
+        dropdownRender={(
+          originNode: ReactElement & { ref?: RefObject<HTMLElement> },
+        ) => {
+          if (isDropdownVisible && !wasDropdownVisible) {
+            originNode.ref?.current?.scrollTo({ top: 0 });
+          }
+          return originNode;
+        }}
+        onFocus={setFocusedFilter}
+        // @ts-ignore
+        onChange={handleChange}
+        ref={inputRef}
+        loading={isRefreshing}
+        maxTagCount={5}
+        menuItemSelectedIcon={<Icon iconSize="m" />}
       >
-        <StyledSelect
-          allowClear
-          // @ts-ignore
-          value={filterState.value || []}
-          disabled={isDisabled}
-          showSearch={showSearch}
-          mode={multiSelect ? 'multiple' : undefined}
-          placeholder={placeholderText}
-          onSearch={searchWrapper}
-          onSelect={clearSuggestionSearch}
-          onBlur={handleBlur}
-          onDropdownVisibleChange={setIsDropdownVisible}
-          dropdownRender={(
-            originNode: ReactElement & { ref?: RefObject<HTMLElement> },
-          ) => {
-            if (isDropdownVisible && !wasDropdownVisible) {
-              originNode.ref?.current?.scrollTo({ top: 0 });
-            }
-            return originNode;
-          }}
-          onFocus={setFocusedFilter}
-          // @ts-ignore
-          onChange={handleChange}
-          ref={inputRef}
-          loading={isRefreshing}
-          maxTagCount={5}
-          menuItemSelectedIcon={<Icon iconSize="m" />}
-        >
-          {sortedData.map(row => {
-            const [value] = groupby.map(col => row[col]);
-            return (
-              // @ts-ignore
-              <Option key={`${value}`} value={value}>
-                {labelFormatter(value, datatype)}
-              </Option>
-            );
-          })}
-          {currentSuggestionSearch &&
-            !ensureIsArray(filterState.value).some(
-              suggestion => suggestion === currentSuggestionSearch,
-            ) && (
-              <Option value={currentSuggestionSearch}>
-                {`${t('Create "%s"', currentSuggestionSearch)}`}
-              </Option>
-            )}
-        </StyledSelect>
-      </FormItem>
+        {sortedData.map(row => {
+          const [value] = groupby.map(col => row[col]);
+          return (
+            // @ts-ignore
+            <Option key={`${value}`} value={value}>
+              {labelFormatter(value, datatype)}
+            </Option>
+          );
+        })}
+        {currentSuggestionSearch &&
+          !ensureIsArray(filterState.value).some(
+            suggestion => suggestion === currentSuggestionSearch,
+          ) && (
+            <Option value={currentSuggestionSearch}>
+              {`${t('Create "%s"', currentSuggestionSearch)}`}
+            </Option>
+          )}
+      </StyledSelect>
     </Styles>
   );
 }
diff --git a/superset-frontend/src/filters/components/common.ts b/superset-frontend/src/filters/components/common.ts
index 1a57f2ae34..5d0046e7ad 100644
--- a/superset-frontend/src/filters/components/common.ts
+++ b/superset-frontend/src/filters/components/common.ts
@@ -21,7 +21,7 @@ import { Select } from 'src/common/components';
 import { PluginFilterStylesProps } from './types';
 
 export const Styles = styled.div<PluginFilterStylesProps>`
-  height: ${({ height }) => height}px;
+  min-height: ${({ height }) => height}px;
   width: ${({ width }) => width}px;
 `;