review comments

elastic · May 31, 2021 · 3849b04 · 3849b04
1 parent f0b137d
commit 3849b04
Show file tree

Hide file tree

Showing 9 changed files with 67 additions and 37 deletions.
diff --git a/src/plugins/data/common/search/aggs/utils/parse_time_shift.ts b/src/plugins/data/common/search/aggs/utils/parse_time_shift.ts
@@ -16,10 +16,11 @@ type AllowedUnit = typeof allowedUnits[number];
  * Allowed values are the string 'previous' and an integer followed by the units s,m,h,d,w,M,y
  *  */
 export const parseTimeShift = (val: string): moment.Duration | 'previous' | 'invalid' => {
-  if (val === 'previous') {
+  const trimmedVal = val.trim();
+  if (trimmedVal === 'previous') {
     return 'previous';
   }
-  const [, amount, unit] = val.match(/^(\d+)(\w)$/) || [];
+  const [, amount, unit] = trimmedVal.match(/^(\d+)(\w)$/) || [];
   const parsedAmount = Number(amount);
   if (Number.isNaN(parsedAmount) || !allowedUnits.includes(unit as AllowedUnit)) {
     return 'invalid';

diff --git a/...plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/...plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx
@@ -19,6 +19,7 @@ import {
   EuiLink,
   EuiPageContentBody,
   EuiButton,
+  EuiSpacer,
 } from '@elastic/eui';
 import { CoreStart, ApplicationStart } from 'kibana/public';
 import {
@@ -452,6 +453,41 @@ export const VisualizationWrapper = ({
     [dispatchLens]
   );
 
+  function renderFixAction(
+    validationError:
+      | {
+          shortMessage: string;
+          longMessage: string;
+          fixAction?:
+            | { label: string; newState: (framePublicAPI: FramePublicAPI) => Promise<unknown> }
+            | undefined;
+        }
+      | undefined
+  ) {
+    return (
+      validationError &&
+      validationError.fixAction &&
+      activeDatasourceId && (
+        <>
+          <EuiButton
+            data-test-subj="errorFixAction"
+            onClick={async () => {
+              const newState = await validationError.fixAction?.newState(framePublicAPI);
+              dispatch({
+                type: 'UPDATE_DATASOURCE_STATE',
+                datasourceId: activeDatasourceId,
+                updater: newState,
+              });
+            }}
+          >
+            {validationError.fixAction.label}
+          </EuiButton>
+          <EuiSpacer />
+        </>
+      )
+    );
+  }
+
   if (localState.configurationValidationError?.length) {
     let showExtraErrors = null;
     let showExtraErrorsAction = null;
@@ -460,14 +496,17 @@ export const VisualizationWrapper = ({
       if (localState.expandError) {
         showExtraErrors = localState.configurationValidationError
           .slice(1)
-          .map(({ longMessage }) => (
-            <p
-              key={longMessage}
-              className="eui-textBreakWord"
-              data-test-subj="configuration-failure-error"
-            >
-              {longMessage}
-            </p>
+          .map((validationError) => (
+            <>
+              <p
+                key={validationError.longMessage}
+                className="eui-textBreakWord"
+                data-test-subj="configuration-failure-error"
+              >
+                {validationError.longMessage}
+              </p>
+              {renderFixAction(validationError)}
+            </>
           ));
       } else {
         showExtraErrorsAction = (
@@ -499,23 +538,7 @@ export const VisualizationWrapper = ({
                 <p className="eui-textBreakWord" data-test-subj="configuration-failure-error">
                   {localState.configurationValidationError[0].longMessage}
                 </p>
-                {localState.configurationValidationError[0].fixAction && activeDatasourceId && (
-                  <EuiButton
-                    data-test-subj="errorFixAction"
-                    onClick={async () => {
-                      const newState = await localState.configurationValidationError?.[0].fixAction?.newState(
-                        framePublicAPI
-                      );
-                      dispatch({
-                        type: 'UPDATE_DATASOURCE_STATE',
-                        datasourceId: activeDatasourceId,
-                        updater: newState,
-                      });
-                    }}
-                  >
-                    {localState.configurationValidationError[0].fixAction.label}
-                  </EuiButton>
-                )}
+                {renderFixAction(localState.configurationValidationError?.[0])}
 
                 {showExtraErrors}
               </>

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/advanced_options.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/advanced_options.tsx
@@ -20,9 +20,7 @@ export function AdvancedOptions(props: {
 }) {
   const [popoverOpen, setPopoverOpen] = useState(false);
   const popoverOptions = props.options.filter((option) => option.showInPopover);
-  const inlineOptions = props.options
-    .filter((option) => option.inlineElement)
-    .map((option) => React.cloneElement(option.inlineElement!, { key: option.dataTestSubj }));
+  const inlineOptions = props.options.filter((option) => option.inlineElement);
 
   return (
     <>
@@ -74,7 +72,12 @@ export function AdvancedOptions(props: {
       {inlineOptions.length > 0 && (
         <>
           <EuiSpacer size="s" />
-          {inlineOptions}
+          {inlineOptions.map((option, index) => (
+            <>
+              {React.cloneElement(option.inlineElement!, { key: option.dataTestSubj })}
+              {index !== inlineOptions.length - 1 && <EuiSpacer size="s" />}
+            </>
+          ))}
         </>
       )}
     </>

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_shift.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/time_shift.tsx
@@ -35,6 +35,7 @@ export function setTimeShift(
   layer: IndexPatternLayer,
   timeShift: string | undefined
 ) {
+  const trimmedTimeShift = timeShift?.trim();
   const currentColumn = layer.columns[columnId];
   const label = currentColumn.customLabel
     ? currentColumn.label
@@ -43,7 +44,7 @@ export function setTimeShift(
         currentColumn.timeScale,
         currentColumn.timeScale,
         currentColumn.timeShift,
-        timeShift
+        trimmedTimeShift
       );
   return {
     ...layer,
@@ -52,7 +53,7 @@ export function setTimeShift(
       [columnId]: {
         ...layer.columns[columnId],
         label,
-        timeShift,
+        timeShift: trimmedTimeShift,
       },
     },
   };
@@ -272,6 +273,7 @@ export function TimeShift({
               onChange={(choices) => {
                 if (choices.length === 0) {
                   updateLayer(setTimeShift(columnId, layer, ''));
+                  setLocalValue('');
                   return;
                 }
 

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/cardinality.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/cardinality.tsx
@@ -84,10 +84,6 @@ export const cardinalityOperation: OperationDefinition<CardinalityIndexPatternCo
   },
   filterable: true,
   shiftable: true,
-  operationParams: [
-    { name: 'kql', type: 'string', required: false },
-    { name: 'lucene', type: 'string', required: false },
-  ],
   getDefaultLabel: (column, indexPattern) =>
     ofName(getSafeName(column.sourceField, indexPattern), column.timeShift),
   buildColumn({ field, previousColumn }, columnParams) {
@@ -109,6 +105,7 @@ export const cardinalityOperation: OperationDefinition<CardinalityIndexPatternCo
       enabled: true,
       schema: 'metric',
       field: column.sourceField,
+      // time shift is added to wrapping aggFilteredMetric if filter is set
       timeShift: column.filter ? undefined : column.timeShift,
     }).toAst();
   },

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/count.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/count.tsx
@@ -102,6 +102,7 @@ export const countOperation: OperationDefinition<CountIndexPatternColumn, 'field
       id: columnId,
       enabled: true,
       schema: 'metric',
+      // time shift is added to wrapping aggFilteredMetric if filter is set
       timeShift: column.filter ? undefined : column.timeShift,
     }).toAst();
   },

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/last_value.tsx
@@ -194,6 +194,7 @@ export const lastValueOperation: OperationDefinition<LastValueIndexPatternColumn
       size: 1,
       sortOrder: 'desc',
       sortField: column.params.sortField,
+      // time shift is added to wrapping aggFilteredMetric if filter is set
       timeShift: column.filter ? undefined : column.timeShift,
     }).toAst();
   },

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/metrics.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/metrics.tsx
@@ -124,6 +124,7 @@ function buildMetricOperation<T extends MetricColumn<string>>({
         enabled: true,
         schema: 'metric',
         field: column.sourceField,
+        // time shift is added to wrapping aggFilteredMetric if filter is set
         timeShift: column.filter ? undefined : column.timeShift,
       }).toAst();
     },

diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx
@@ -130,6 +130,7 @@ export const percentileOperation: OperationDefinition<PercentileIndexPatternColu
         schema: 'metric',
         field: column.sourceField,
         percentile: column.params.percentile,
+        // time shift is added to wrapping aggFilteredMetric if filter is set
         timeShift: column.filter ? undefined : column.timeShift,
       }
     ).toAst();