[TIP] Fix many small UI issues

- add EuiTooltip for all EuiButtonIcon - add missing translations - replace css with EuiFlexGroup where possible - extract fieldValueValid logic
elastic · Sep 19, 2022 · eb5538a · eb5538a
1 parent 5af030b
commit eb5538a
Show file tree

Hide file tree

Showing 21 changed files with 329 additions and 257 deletions.
diff --git a/...dicators/components/indicator_barchart_legend_action/indicator_barchart_legend_action.tsx b/...dicators/components/indicator_barchart_legend_action/indicator_barchart_legend_action.tsx
@@ -6,7 +6,8 @@
  */
 
 import React, { useState, VFC } from 'react';
-import { EuiButtonIcon, EuiContextMenuPanel, EuiPopover } from '@elastic/eui';
+import { EuiButtonIcon, EuiContextMenuPanel, EuiPopover, EuiToolTip } from '@elastic/eui';
+import { i18n } from '@kbn/i18n';
 import { ComponentType } from '../../../../../common/types/component_type';
 import { FilterIn } from '../../../query_bar/components/filter_in';
 import { FilterOut } from '../../../query_bar/components/filter_out';
@@ -17,6 +18,10 @@ export const TIMELINE_BUTTON_TEST_ID = 'tiBarchartTimelineButton';
 export const FILTER_IN_BUTTON_TEST_ID = 'tiBarchartFilterInButton';
 export const FILTER_OUT_BUTTON_TEST_ID = 'tiBarchartFilterOutButton';
 
+const BUTTON_LABEL = i18n.translate('xpack.threatIntelligence.indicator.barChart.popover', {
+  defaultMessage: 'More actions',
+});
+
 export interface IndicatorBarchartLegendActionProps {
   /**
    * Indicator
@@ -59,12 +64,15 @@ export const IndicatorBarchartLegendAction: VFC<IndicatorBarchartLegendActionPro
     <EuiPopover
       data-test-subj={POPOVER_BUTTON_TEST_ID}
       button={
-        <EuiButtonIcon
-          iconType="boxesHorizontal"
-          iconSize="s"
-          size="xs"
-          onClick={() => setPopover(!isPopoverOpen)}
-        />
+        <EuiToolTip content={BUTTON_LABEL}>
+          <EuiButtonIcon
+            aria-label={BUTTON_LABEL}
+            iconType="boxesHorizontal"
+            iconSize="s"
+            size="xs"
+            onClick={() => setPopover(!isPopoverOpen)}
+          />
+        </EuiToolTip>
       }
       isOpen={isPopoverOpen}
       closePopover={() => setPopover(false)}

diff --git a/.../public/modules/indicators/components/indicator_value_actions/indicator_value_actions.tsx b/.../public/modules/indicators/components/indicator_value_actions/indicator_value_actions.tsx
@@ -7,12 +7,12 @@
 
 import type { EuiButtonEmpty, EuiButtonIcon } from '@elastic/eui';
 import React, { VFC } from 'react';
-import { EMPTY_VALUE } from '../../../../../common/constants';
+import { EuiFlexGroup } from '@elastic/eui';
 import { Indicator } from '../../../../../common/types/indicator';
 import { FilterIn } from '../../../query_bar/components/filter_in';
 import { FilterOut } from '../../../query_bar/components/filter_out';
 import { AddToTimeline } from '../../../timeline/components/add_to_timeline';
-import { getIndicatorFieldAndValue } from '../../lib/field_value';
+import { fieldAndValueValid, getIndicatorFieldAndValue } from '../../lib/field_value';
 
 export const TIMELINE_BUTTON_TEST_ID = 'TimelineButton';
 export const FILTER_IN_BUTTON_TEST_ID = 'FilterInButton';
@@ -44,8 +44,7 @@ export const IndicatorValueActions: VFC<IndicatorValueActions> = ({
   ...props
 }) => {
   const { key, value } = getIndicatorFieldAndValue(indicator, field);
-
-  if (!key || value === EMPTY_VALUE || !key) {
+  if (!fieldAndValueValid(key, value)) {
     return null;
   }
 
@@ -54,7 +53,7 @@ export const IndicatorValueActions: VFC<IndicatorValueActions> = ({
   const timelineTestId = `${props['data-test-subj']}${TIMELINE_BUTTON_TEST_ID}`;
 
   return (
-    <>
+    <EuiFlexGroup justifyContent="center" alignItems="center">
       <FilterIn as={Component} data={indicator} field={field} data-test-subj={filterInTestId} />
       <FilterOut as={Component} data={indicator} field={field} data-test-subj={filterOutTestId} />
       <AddToTimeline
@@ -63,6 +62,6 @@ export const IndicatorValueActions: VFC<IndicatorValueActions> = ({
         field={field}
         data-test-subj={timelineTestId}
       />
-    </>
+    </EuiFlexGroup>
   );
 };
diff --git a/...ts/indicators_flyout/tabs/indicators_flyout_overview/components/block/indicator_block.tsx b/...ts/indicators_flyout/tabs/indicators_flyout_overview/components/block/indicator_block.tsx
@@ -25,7 +25,7 @@ const VisibleOnHover = euiStyled.div`
     & .actionsWrapper {
       visibility: hidden;
       display: inline-block;
-      margin-inline-start: ${theme.eui.euiSizeXS};
+      margin-inline-start: ${theme.eui.euiSizeS};
     }
 
     &:hover .actionsWrapper {

diff --git a/...t_intelligence/public/modules/indicators/components/indicators_table/actions_row_cell.tsx b/...t_intelligence/public/modules/indicators/components/indicators_table/actions_row_cell.tsx
@@ -6,17 +6,15 @@
  */
 
 import React, { useContext, VFC } from 'react';
+import { EuiFlexGroup } from '@elastic/eui';
 import { InvestigateInTimelineButtonIcon } from '../../../timeline/components/investigate_in_timeline_button_icon';
 import { Indicator } from '../../../../../common/types/indicator';
 import { OpenIndicatorFlyoutButton } from '../open_indicator_flyout_button/open_indicator_flyout_button';
 import { IndicatorsTableContext } from './context';
-import { useStyles } from './styles';
 
 const INVESTIGATE_TEST_ID = 'tiIndicatorTableInvestigateInTimelineButtonIcon';
 
 export const ActionsRowCell: VFC<{ indicator: Indicator }> = ({ indicator }) => {
-  const styles = useStyles();
-
   const indicatorTableContext = useContext(IndicatorsTableContext);
 
   if (!indicatorTableContext) {
@@ -26,13 +24,13 @@ export const ActionsRowCell: VFC<{ indicator: Indicator }> = ({ indicator }) =>
   const { setExpanded, expanded } = indicatorTableContext;
 
   return (
-    <div css={styles.rowActionsDiv}>
+    <EuiFlexGroup justifyContent="center">
       <OpenIndicatorFlyoutButton
         indicator={indicator}
         onOpen={setExpanded}
         isOpen={Boolean(expanded && expanded._id === indicator._id)}
       />
       <InvestigateInTimelineButtonIcon data={indicator} data-test-subj={INVESTIGATE_TEST_ID} />
-    </div>
+    </EuiFlexGroup>
   );
 };
diff --git a/...hreat_intelligence/public/modules/indicators/components/indicators_table/cell_actions.tsx b/...hreat_intelligence/public/modules/indicators/components/indicators_table/cell_actions.tsx
@@ -8,11 +8,10 @@
 import React, { VFC } from 'react';
 import { EuiDataGridColumnCellActionProps } from '@elastic/eui/src/components/datagrid/data_grid_types';
 import { ComponentType } from '../../../../../common/types/component_type';
-import { EMPTY_VALUE } from '../../../../../common/constants';
 import { Indicator } from '../../../../../common/types/indicator';
 import { Pagination } from '../../hooks/use_indicators';
 import { AddToTimeline } from '../../../timeline/components/add_to_timeline';
-import { getIndicatorFieldAndValue } from '../../lib/field_value';
+import { fieldAndValueValid, getIndicatorFieldAndValue } from '../../lib/field_value';
 import { FilterIn } from '../../../query_bar/components/filter_in';
 import { FilterOut } from '../../../query_bar/components/filter_out';
 
@@ -47,8 +46,7 @@ export const CellActions: VFC<CellActionsProps> = ({
 }) => {
   const indicator = indicators[rowIndex % pagination.pageSize];
   const { key, value } = getIndicatorFieldAndValue(indicator, columnId);
-
-  if (!value || value === EMPTY_VALUE || !key) {
+  if (!fieldAndValueValid(key, value)) {
     return <></>;
   }
 

diff --git a/...ugins/threat_intelligence/public/modules/indicators/components/indicators_table/styles.ts b/...ugins/threat_intelligence/public/modules/indicators/components/indicators_table/styles.ts
diff --git a/...dules/indicators/components/open_indicator_flyout_button/open_indicator_flyout_button.tsx b/...dules/indicators/components/open_indicator_flyout_button/open_indicator_flyout_button.tsx
@@ -12,6 +12,13 @@ import { Indicator } from '../../../../../common/types/indicator';
 
 export const BUTTON_TEST_ID = 'tiToggleIndicatorFlyoutButton';
 
+const BUTTON_LABEL: string = i18n.translate(
+  'xpack.threatIntelligence.indicator.table.viewDetailsButton',
+  {
+    defaultMessage: 'View details',
+  }
+);
+
 export interface OpenIndicatorFlyoutButtonProps {
   /**
    * {@link Indicator} passed to the flyout component.
@@ -35,22 +42,15 @@ export const OpenIndicatorFlyoutButton: VFC<OpenIndicatorFlyoutButtonProps> = ({
   onOpen,
   isOpen,
 }) => {
-  const buttonLabel: string = i18n.translate(
-    'xpack.threatIntelligence.indicator.table.viewDetailsButton',
-    {
-      defaultMessage: 'View details',
-    }
-  );
-
   return (
-    <EuiToolTip content={buttonLabel} delay="long">
+    <EuiToolTip content={BUTTON_LABEL}>
       <EuiButtonIcon
         data-test-subj={BUTTON_TEST_ID}
         color={isOpen ? 'text' : 'primary'}
         iconType={isOpen ? 'minimize' : 'expand'}
         isSelected={isOpen}
         iconSize="s"
-        aria-label={buttonLabel}
+        aria-label={BUTTON_LABEL}
         onClick={() => onOpen(indicator)}
       />
     </EuiToolTip>

diff --git a/x-pack/plugins/threat_intelligence/public/modules/indicators/lib/field_value.test.ts b/x-pack/plugins/threat_intelligence/public/modules/indicators/lib/field_value.test.ts
@@ -5,39 +5,76 @@
  * 2.0.
  */
 
-import { getIndicatorFieldAndValue } from './field_value';
+import { fieldAndValueValid, getIndicatorFieldAndValue } from './field_value';
 import {
   generateMockFileIndicator,
   generateMockUrlIndicator,
 } from '../../../../common/types/indicator';
+import { EMPTY_VALUE } from '../../../../common/constants';
 
-describe('getIndicatorFieldAndValue()', () => {
-  it('should return field/value pair for an indicator', () => {
-    const mockData = generateMockUrlIndicator();
-    const mockKey = 'threat.feed.name';
+describe('field_value', () => {
+  describe('getIndicatorFieldAndValue()', () => {
+    it('should return field/value pair for an indicator', () => {
+      const mockData = generateMockUrlIndicator();
+      const mockKey = 'threat.feed.name';
 
-    const result = getIndicatorFieldAndValue(mockData, mockKey);
-    expect(result.key).toEqual(mockKey);
-    expect(result.value).toEqual((mockData.fields[mockKey] as unknown as string[])[0]);
-  });
+      const result = getIndicatorFieldAndValue(mockData, mockKey);
+      expect(result.key).toEqual(mockKey);
+      expect(result.value).toEqual((mockData.fields[mockKey] as unknown as string[])[0]);
+    });
+
+    it('should return a null value for an incorrect field', () => {
+      const mockData = generateMockUrlIndicator();
+      const mockKey = 'abc';
+
+      const result = getIndicatorFieldAndValue(mockData, mockKey);
+      expect(result.key).toEqual(mockKey);
+      expect(result.value).toBeNull();
+    });
 
-  it('should return a null value for an incorrect field', () => {
-    const mockData = generateMockUrlIndicator();
-    const mockKey = 'abc';
+    it('should return field/value pair for an indicator and DisplayName field', () => {
+      const mockData = generateMockFileIndicator();
+      const mockKey = 'threat.indicator.name';
 
-    const result = getIndicatorFieldAndValue(mockData, mockKey);
-    expect(result.key).toEqual(mockKey);
-    expect(result.value).toBeNull();
+      const result = getIndicatorFieldAndValue(mockData, mockKey);
+      expect(result.key).toEqual(
+        (mockData.fields['threat.indicator.name_origin'] as unknown as string[])[0]
+      );
+      expect(result.value).toEqual((mockData.fields[mockKey] as unknown as string[])[0]);
+    });
   });
 
-  it('should return field/value pair for an indicator and DisplayName field', () => {
-    const mockData = generateMockFileIndicator();
-    const mockKey = 'threat.indicator.name';
+  describe('fieldAndValueValid()', () => {
+    it('should return false for null value', () => {
+      const mockField = 'abc';
+      const mockValue = null;
+
+      const result = fieldAndValueValid(mockField, mockValue);
+      expect(result).toEqual(false);
+    });
+
+    it(`should return false for ${EMPTY_VALUE} value`, () => {
+      const mockField = 'abc';
+      const mockValue = EMPTY_VALUE;
+
+      const result = fieldAndValueValid(mockField, mockValue);
+      expect(result).toEqual(false);
+    });
+
+    it('should return false for empty field', () => {
+      const mockField = '';
+      const mockValue = 'abc';
+
+      const result = fieldAndValueValid(mockField, mockValue);
+      expect(result).toEqual(false);
+    });
+
+    it('should return true if field and value are correct', () => {
+      const mockField = 'abc';
+      const mockValue = 'abc';
 
-    const result = getIndicatorFieldAndValue(mockData, mockKey);
-    expect(result.key).toEqual(
-      (mockData.fields['threat.indicator.name_origin'] as unknown as string[])[0]
-    );
-    expect(result.value).toEqual((mockData.fields[mockKey] as unknown as string[])[0]);
+      const result = fieldAndValueValid(mockField, mockValue);
+      expect(result).toEqual(true);
+    });
   });
 });
diff --git a/x-pack/plugins/threat_intelligence/public/modules/indicators/lib/field_value.ts b/x-pack/plugins/threat_intelligence/public/modules/indicators/lib/field_value.ts
@@ -5,6 +5,7 @@
  * 2.0.
  */
 
+import { EMPTY_VALUE } from '../../../../common/constants';
 import { unwrapValue } from './unwrap_value';
 import { Indicator, RawIndicatorFieldId } from '../../../../common/types/indicator';
 
@@ -29,3 +30,12 @@ export const getIndicatorFieldAndValue = (
     value,
   };
 };
+
+/**
+ * Checks if field and value are correct
+ * @param field Indicator string field
+ * @param value Indicator string|null value for the field
+ * @returns true if correct, false if not
+ */
+export const fieldAndValueValid = (field: string | null, value: string | null): boolean =>
+  value != null && value !== '' && value !== EMPTY_VALUE && field != null && field !== '';