Simplify identification of each aggregation element in aggregation builder. (#10387)

* Display background for each element section instead of each element.

* Display add section button as icon next to element headline

* Cleaning up previous changes

* Improving naming of add element section button title.

* Removing no longer needed "Add a sort" button.

* Removing no longer needed import.

* Renaming `addEmptySection` to `addEmptyElement`.

* Renaming `allowAddSection` to `allowAddEmptyElement`.

* Renaming `onAddElementSection` to `onAddEmptyElement`.

* Implementing proper theme colors for recent aggregation builder layout changes.

* Add gap between aggregation builder controls and visualization.

* Add border for element configuration form containers.

* Fixing style linter warnings.

* Fixing remaining style linter warnings.

Author: linuspahl

graylog2-web-interface/src/views/components/aggregationwizard/AggregationWizard.tsx

@@ -41,9 +41,10 @@ const Wrapper = styled.div`
 
 const Controls = styled.div`
   height: 100%;
-  min-width: 300px;
+  min-width: 315px;
   max-width: 500px;
-  flex: 1;
+  flex: 1.2;
+  padding-right: 15px;
   overflow-y: auto;
 `;
 
@@ -60,7 +61,7 @@ const Section = styled.div`
   }
 `;
 
-const _onElementCreate = (
+const onAddEmptyElement = (
   elementKey: string,
   values: WidgetConfigFormValues,
   setValues: (formValues: WidgetConfigFormValues) => void,
@@ -116,13 +117,14 @@ const AggregationWizard = ({ onChange, config, children }: EditWidgetComponentPr
           {({ values, setValues }) => (
             <>
               <Section data-testid="add-element-section">
-                <AggregationElementSelect onElementCreate={(elementKey) => _onElementCreate(elementKey, values, setValues)}
+                <AggregationElementSelect onElementCreate={(elementKey) => onAddEmptyElement(elementKey, values, setValues)}
                                           aggregationElements={aggregationElements}
                                           formValues={values} />
               </Section>
               <Section data-testid="configure-elements-section">
                 <ElementsConfiguration aggregationElementsByKey={aggregationElementsByKey}
                                        config={config}
+                                       onAddEmptyElement={onAddEmptyElement}
                                        onConfigChange={onChange} />
               </Section>
             </>

graylog2-web-interface/src/views/components/aggregationwizard/ElementsConfiguration.tsx

@@ -43,19 +43,16 @@ type Props = {
   aggregationElementsByKey: { [elementKey: string]: AggregationElement }
   config: AggregationWidgetConfig,
   onConfigChange: (config: AggregationWidgetConfig) => void,
+  onAddEmptyElement: (
+    elementKey: string,
+    values: WidgetConfigFormValues,
+    setValues: (formValues: WidgetConfigFormValues) => void,
+  ) => void,
 }
 
-const ElementsConfiguration = ({ aggregationElementsByKey, config, onConfigChange }: Props) => {
+const ElementsConfiguration = ({ aggregationElementsByKey, config, onConfigChange, onAddEmptyElement }: Props) => {
   const { values, setValues, dirty } = useFormikContext<WidgetConfigFormValues>();
 
-  const _onDeleteElement = (aggregationElement) => {
-    if (typeof aggregationElement.onDeleteAll !== 'function') {
-      return;
-    }
-
-    setValues(aggregationElement.onDeleteAll(values));
-  };
-
   return (
     <Container>
       <div>
@@ -73,9 +70,10 @@ const ElementsConfiguration = ({ aggregationElementsByKey, config, onConfigChang
           const AggregationElementComponent = aggregationElement.component;
 
           return (
-            <ElementConfigurationContainer isPermanentElement={aggregationElement.onDeleteAll === undefined}
+            <ElementConfigurationContainer allowAddEmptyElement={aggregationElement.allowCreate(values)}
                                            title={aggregationElement.title}
-                                           onDeleteAll={() => _onDeleteElement(aggregationElement)}
+                                           titleSingular={aggregationElement.titleSingular}
+                                           onAddEmptyElement={() => onAddEmptyElement(aggregationElement.key, values, setValues)}
                                            key={aggregationElement.key}>
               <AggregationElementComponent config={config} onConfigChange={onConfigChange} />
             </ElementConfigurationContainer>

graylog2-web-interface/src/views/components/aggregationwizard/ElementsConfigurationActions.tsx

@@ -27,12 +27,12 @@ import type { WidgetConfigFormValues } from './WidgetConfigForm';
 const ConfigActions = styled.div<{ scrolledToBottom: boolean }>(({ theme, scrolledToBottom }) => css`
   position: sticky;
   width: 100%;
-  bottom: 0px;
+  bottom: 0;
   padding-top: 5px;
   background: ${theme.colors.global.contentBackground};
   z-index: 1;
 
-  :before {
+  ::before {
     box-shadow: 1px -2px 3px rgb(0 0 0 / 25%);
     content: ' ';
     display: ${scrolledToBottom ? 'block' : 'none'};
@@ -47,7 +47,7 @@ const ConfigActions = styled.div<{ scrolledToBottom: boolean }>(({ theme, scroll
 const ScrolledToBottomIndicator = styled.div`
   width: 100%;
   position: absolute;
-  bottom: 0px;
+  bottom: 0;
   height: 5px;
   z-index: 0;
 `;

graylog2-web-interface/src/views/components/aggregationwizard/aggregationElements/AggregationElementType.ts

@@ -20,6 +20,7 @@ import type { WidgetConfigFormValues, WidgetConfigValidationErrors } from '../Wi
 
 export type AggregationElement = {
   title: string,
+  titleSingular?: string,
   key: string,
   allowCreate: (formValues: WidgetConfigFormValues) => boolean,
   order: number,

graylog2-web-interface/src/views/components/aggregationwizard/aggregationElements/GroupByElement.ts

@@ -177,6 +177,7 @@ export const emptyGrouping: ValuesGrouping = {
 
 const GroupByElement: AggregationElement = {
   title: 'Group By',
+  titleSingular: 'Grouping',
   key: 'groupBy',
   order: 1,
   allowCreate: () => true,

graylog2-web-interface/src/views/components/aggregationwizard/elementConfiguration/ElementConfigurationContainer.test.tsx

@@ -22,8 +22,8 @@ import ElementConfigurationContainer from './ElementConfigurationContainer';
 describe('ElementConfigurationContainer', () => {
   it('should render elements passed as children', () => {
     render(
-      <ElementConfigurationContainer isPermanentElement={false}
-                                     onDeleteAll={() => {}}
+      <ElementConfigurationContainer allowAddEmptyElement
+                                     onAddEmptyElement={() => {}}
                                      title="Aggregation Element Title">
         Children of Dune
       </ElementConfigurationContainer>,
@@ -34,8 +34,8 @@ describe('ElementConfigurationContainer', () => {
 
   it('should render title', () => {
     render(
-      <ElementConfigurationContainer isPermanentElement={false}
-                                     onDeleteAll={() => {}}
+      <ElementConfigurationContainer allowAddEmptyElement
+                                     onAddEmptyElement={() => {}}
                                      title="Aggregation Element Title">
         Children of Dune
       </ElementConfigurationContainer>,
@@ -44,33 +44,33 @@ describe('ElementConfigurationContainer', () => {
     expect(screen.getByText('Aggregation Element Title')).toBeInTheDocument();
   });
 
-  it('should call on delete when clicking delete icon', async () => {
-    const onDeleteAllMock = jest.fn();
+  it('should call on onAddEmptyElement when adding a section', async () => {
+    const onAddEmptyElementMock = jest.fn();
 
     render(
-      <ElementConfigurationContainer isPermanentElement={false}
-                                     onDeleteAll={onDeleteAllMock}
+      <ElementConfigurationContainer allowAddEmptyElement
+                                     onAddEmptyElement={onAddEmptyElementMock}
                                      title="Aggregation Element Title">
         Children of Dune
       </ElementConfigurationContainer>,
     );
 
-    const deleteButton = screen.getByTitle('Remove Aggregation Element Title');
+    const addButton = screen.getByTitle('Add a Aggregation Element Title');
 
-    fireEvent.click(deleteButton);
+    fireEvent.click(addButton);
 
-    expect(onDeleteAllMock).toHaveBeenCalledTimes(1);
+    expect(onAddEmptyElementMock).toHaveBeenCalledTimes(1);
   });
 
-  it('should not display delete icon if element is permanent', async () => {
+  it('should not display add section icon if adding element section is not allowed', async () => {
     render(
-      <ElementConfigurationContainer isPermanentElement
-                                     onDeleteAll={() => {}}
+      <ElementConfigurationContainer allowAddEmptyElement={false}
+                                     onAddEmptyElement={() => {}}
                                      title="Aggregation Element Title">
         Children of Dune
       </ElementConfigurationContainer>,
     );
 
-    expect(screen.queryByTitle('Remove Aggregation Element Title')).not.toBeInTheDocument();
+    expect(screen.queryByTitle('Add a Aggregation Element Title')).not.toBeInTheDocument();
   });
 });

graylog2-web-interface/src/views/components/aggregationwizard/elementConfiguration/ElementConfigurationContainer.tsx

@@ -20,11 +20,8 @@ import styled, { css } from 'styled-components';
 import IconButton from 'components/common/IconButton';
 
 const Wrapper = styled.div(({ theme }) => css`
-  background-color: ${theme.colors.variant.lightest.default};
-  border: 1px solid ${theme.colors.variant.lighter.default};
-  padding: 6px 6px 3px 6px;
-  border-radius: 6px;
   margin-bottom: 6px;
+  border-radius: 6px;
 
   :last-child {
     margin-bottom: 0;
@@ -56,34 +53,66 @@ const Wrapper = styled.div(({ theme }) => css`
   }
 `);
 
-const Header = styled.div`
+const Header = styled.div(({ theme }) => css`
   display: flex;
   justify-content: space-between;
-  margin-bottom: 5px;
-`;
+  align-items: center;
+  margin-bottom: 1px;
+  min-height: 26px;
+  font-weight: bold;
+  position: relative;
+
+  ::before {
+    content: ' ';
+    top: 50%;
+    width: 100%;
+    border-bottom: 1px solid ${theme.utils.contrastingColor(theme.colors.global.contentBackground, 'AA')};
+    position: absolute;
+  }
+`);
+
+const ElementTitle = styled.div(({ theme }) => css`
+  background-color: ${theme.colors.global.contentBackground};
+  z-index: 1;
+  padding-right: 8px;
+`);
+
+const ElementActions = styled.div(({ theme }) => css`
+  background-color: ${theme.colors.global.contentBackground};
+  z-index: 1;
+  padding-left: 5px;
+`);
+
+const StyledIconButton = styled(IconButton)(({ theme }) => `
+  color: ${theme.colors.global.textDefault};
+`);
 
 type Props = {
+  allowAddEmptyElement: boolean,
   children: React.ReactNode,
-  isPermanentElement: boolean,
-  onDeleteAll: () => void
+  onAddEmptyElement: () => void,
   title: string,
+  titleSingular?: string,
 }
 
 const ElementConfigurationContainer = ({
   children,
-  isPermanentElement,
-  onDeleteAll,
+  allowAddEmptyElement,
+  onAddEmptyElement,
   title,
+  titleSingular,
 }: Props) => {
   return (
     <Wrapper>
       <Header>
-        <div>{title}</div>
-        <div>
-          {!isPermanentElement && (
-            <IconButton title={`Remove ${title}`} name="trash" onClick={onDeleteAll} />
+        <ElementTitle>
+          {title}
+        </ElementTitle>
+        <ElementActions>
+          {allowAddEmptyElement && (
+            <StyledIconButton title={`Add a ${titleSingular ?? title}`} name="plus" onClick={onAddEmptyElement} />
           )}
-        </div>
+        </ElementActions>
       </Header>
       <div>
         {children}
@@ -92,4 +121,8 @@ const ElementConfigurationContainer = ({
   );
 };
 
+ElementConfigurationContainer.defaultProps = {
+  titleSingular: undefined,
+};
+
 export default ElementConfigurationContainer;

graylog2-web-interface/src/views/components/aggregationwizard/elementConfiguration/ElementConfigurationSection.tsx

@@ -20,11 +20,13 @@ import styled, { css } from 'styled-components';
 import { IconButton } from 'components/common';
 
 const SectionContainer = styled.div(({ theme }) => css`
-  border-bottom: 1px solid ${theme.colors.variant.lighter.default};
+  background-color: ${theme.colors.variant.lightest.default};
   margin-bottom: 5px;
+  padding: 6px 6px 3px 6px;
+  border-radius: 3px;
+  border: 1px solid ${theme.colors.variant.lighter.default};
 
   :last-of-type {
-    border-bottom: 0;
     margin-bottom: 0;
   }
 `);

graylog2-web-interface/src/views/components/aggregationwizard/elementConfiguration/GroupByConfiguration.tsx

@@ -20,21 +20,14 @@ import { useFormikContext, FieldArray, Field } from 'formik';
 import styled from 'styled-components';
 
 import { HoverForHelp } from 'components/common';
-import { Button, ButtonToolbar, Checkbox } from 'components/graylog';
+import { Checkbox } from 'components/graylog';
 
 import ElementConfigurationSection from './ElementConfigurationSection';
 import GroupBy from './GroupBy';
 
-import GroupByElement, { emptyGrouping } from '../aggregationElements/GroupByElement';
+import GroupByElement from '../aggregationElements/GroupByElement';
 import { WidgetConfigFormValues } from '../WidgetConfigForm';
 
-const ActionsBar = styled.div`
-  display: flex;
-  justify-content: space-between;
-  align-items: center;
-  margin-bottom: 3px;
-`;
-
 const RollupColumnsLabel = styled.div`
   display: flex;
   align-items: center;
@@ -53,8 +46,22 @@ const GroupByConfiguration = () => {
 
   return (
     <>
+      <Field name="groupBy.columnRollup">
+        {({ field: { name, onChange, value } }) => (
+          <Checkbox onChange={() => onChange({ target: { name, value: !groupBy.columnRollup } })}
+                    checked={value}
+                    disabled={disableColumnRollup}>
+            <RollupColumnsLabel>
+              Rollup Columns
+              <RollupHoverForHelp title="Rollup Columns">
+                When rollup is enabled, an additional trace totalling individual subtraces will be included.
+              </RollupHoverForHelp>
+            </RollupColumnsLabel>
+          </Checkbox>
+        )}
+      </Field>
       <FieldArray name="groupBy.groupings"
-                  render={(arrayHelpers) => (
+                  render={() => (
                     <>
                       <div>
                         {groupBy.groupings.map((grouping, index) => {
@@ -66,27 +73,6 @@ const GroupByConfiguration = () => {
                           );
                         })}
                       </div>
-                      <ActionsBar>
-                        <Field name="groupBy.columnRollup">
-                          {({ field: { name, onChange, value } }) => (
-                            <Checkbox onChange={() => onChange({ target: { name, value: !groupBy.columnRollup } })}
-                                      checked={value}
-                                      disabled={disableColumnRollup}>
-                              <RollupColumnsLabel>
-                                Rollup Columns
-                                <RollupHoverForHelp title="Rollup Columns">
-                                  When rollup is enabled, an additional trace totalling individual subtraces will be included.
-                                </RollupHoverForHelp>
-                              </RollupColumnsLabel>
-                            </Checkbox>
-                          )}
-                        </Field>
-                        <ButtonToolbar>
-                          <Button className="pull-right" bsSize="small" type="button" onClick={() => arrayHelpers.push(emptyGrouping)}>
-                            Add Grouping
-                          </Button>
-                        </ButtonToolbar>
-                      </ActionsBar>
                     </>
                   )} />
     </>