[Fixes #1212] Confusing sorting UI in Topics (#1389)

* Topics: Updated tests for TableHeaderCell and recreated snapshots * Topics: Updated snapshot * Topics: Added more tests to verify that TableHeaderCell is rendering correctly * Update kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx Co-authored-by: Oleg Shur <workshur@gmail.com> * Update kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx Co-authored-by: Oleg Shur <workshur@gmail.com> * Updates snapshots * Updates snapshot * Updates TableHeaderCell test to use theme object instead of hardcoded values * got rid of codesmells * Adds more tests to raise coverage * cleanup * Removes orderBy, orderValue, otherOrderValue from test * Renames sortIconTitle -> sortIconTitleValue * Renames th -> columnheader in test * Renames title -> testTitle, previewText -> testPreviewText, sortIconTitleValue -> sortIconTitle * Renames titleNode -> title, previewNode -> preview Co-authored-by: Damir Abdulganiev <ehave@Damirs-MacBook-Pro.local> Co-authored-by: Damir Abdulganiev <dabdulganiev@provectus.com> Co-authored-by: Oleg Shur <workshur@gmail.com>
2022-01-19 12:43:00 +03:00 · 2022-01-19 12:43:00 +03:00 · cdc929add7
commit cdc929add7
parent 439d41da0b
8 changed files with 250 additions and 170 deletions
--- a/kafka-ui-react-app/src/components/Connect/Details/Tasks/tests/snapshots/Tasks.spec.tsx.snap
+++ b/kafka-ui-react-app/src/components/Connect/Details/Tasks/tests/snapshots/Tasks.spec.tsx.snap
@ -18,22 +18,7 @@ exports[`Tasks view matches snapshot 1`] = `
  background-color: #F1F2F3;
 }

-.c1 {
-  padding: 4px 0 4px 24px !important;
-  border-bottom-width: 1px !important;
-  vertical-align: middle !important;
-}
-
-.c1.is-clickable {
-  cursor: pointer !important;
-  pointer-events: all !important;
-}
-
-.c1.has-text-link-dark span {
-  color: #4F4FFF !important;
-}
-
-.c1 span {
+.c2 {
  font-family: Inter,sans-serif;
  font-size: 12px;
  font-style: normal;
@ -46,18 +31,13 @@ exports[`Tasks view matches snapshot 1`] = `
  text-align: left;
  background: #FFFFFF;
  color: #73848C;
+  cursor: default;
 }

-.c1 span.preview {
-  margin-left: 8px;
-  font-size: 14px;
-  color: #4F4FFF;
-  cursor: pointer;
-}
-
-.c1 span.is-clickable {
-  cursor: pointer !important;
-  pointer-events: all !important;
+.c1 {
+  padding: 4px 0 4px 24px;
+  border-bottom-width: 1px;
+  vertical-align: middle;
 }

 <table
@ -67,40 +47,36 @@ exports[`Tasks view matches snapshot 1`] = `
    <tr>
      <th
        className="c1"
-        title="ID"
      >
        <span
-          className="title"
+          className="c2"
        >
          ID
        </span>
      </th>
      <th
        className="c1"
-        title="Worker"
      >
        <span
-          className="title"
+          className="c2"
        >
          Worker
        </span>
      </th>
      <th
        className="c1"
-        title="State"
      >
        <span
-          className="title"
+          className="c2"
        >
          State
        </span>
      </th>
      <th
        className="c1"
-        title="Trace"
      >
        <span
-          className="title"
+          className="c2"
        >
          Trace
        </span>
@ -109,7 +85,7 @@ exports[`Tasks view matches snapshot 1`] = `
        className="c1"
      >
        <span
-          className="title"
+          className="c2"
        />
      </th>
    </tr>
@ -203,22 +179,7 @@ exports[`Tasks view matches snapshot when no tasks 1`] = `
  background-color: #F1F2F3;
 }

-.c1 {
-  padding: 4px 0 4px 24px !important;
-  border-bottom-width: 1px !important;
-  vertical-align: middle !important;
-}
-
-.c1.is-clickable {
-  cursor: pointer !important;
-  pointer-events: all !important;
-}
-
-.c1.has-text-link-dark span {
-  color: #4F4FFF !important;
-}
-
-.c1 span {
+.c2 {
  font-family: Inter,sans-serif;
  font-size: 12px;
  font-style: normal;
@ -231,18 +192,13 @@ exports[`Tasks view matches snapshot when no tasks 1`] = `
  text-align: left;
  background: #FFFFFF;
  color: #73848C;
+  cursor: default;
 }

-.c1 span.preview {
-  margin-left: 8px;
-  font-size: 14px;
-  color: #4F4FFF;
-  cursor: pointer;
-}
-
-.c1 span.is-clickable {
-  cursor: pointer !important;
-  pointer-events: all !important;
+.c1 {
+  padding: 4px 0 4px 24px;
+  border-bottom-width: 1px;
+  vertical-align: middle;
 }

 <table
@ -252,40 +208,36 @@ exports[`Tasks view matches snapshot when no tasks 1`] = `
    <tr>
      <th
        className="c1"
-        title="ID"
      >
        <span
-          className="title"
+          className="c2"
        >
          ID
        </span>
      </th>
      <th
        className="c1"
-        title="Worker"
      >
        <span
-          className="title"
+          className="c2"
        >
          Worker
        </span>
      </th>
      <th
        className="c1"
-        title="State"
      >
        <span
-          className="title"
+          className="c2"
        >
          State
        </span>
      </th>
      <th
        className="c1"
-        title="Trace"
      >
        <span
-          className="title"
+          className="c2"
        >
          Trace
        </span>
@ -294,7 +246,7 @@ exports[`Tasks view matches snapshot when no tasks 1`] = `
        className="c1"
      >
        <span
-          className="title"
+          className="c2"
        />
      </th>
    </tr>
--- a/kafka-ui-react-app/src/components/Connect/List/tests/snapshots/ListItem.spec.tsx.snap
+++ b/kafka-ui-react-app/src/components/Connect/List/tests/snapshots/ListItem.spec.tsx.snap
@ -260,6 +260,8 @@ exports[`Connectors ListItem matches snapshot 1`] = `
          "normal": "#FFFFFF",
        },
        "color": Object {
+          "active": "#4F4FFF",
+          "hover": "#4F4FFF",
          "normal": "#73848C",
        },
        "previewColor": Object {
--- a/kafka-ui-react-app/src/components/Topics/Topic/Details/test/snapshots/Details.spec.tsx.snap
+++ b/kafka-ui-react-app/src/components/Topics/Topic/Details/test/snapshots/Details.spec.tsx.snap
@ -271,6 +271,8 @@ exports[`Details when it has readonly flag does not render the Action button a T
          "normal": "#FFFFFF",
        },
        "color": Object {
+          "active": "#4F4FFF",
+          "hover": "#4F4FFF",
          "normal": "#73848C",
        },
        "previewColor": Object {
--- a/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.styled.ts
+++ b/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.styled.ts
@ -1,43 +1,52 @@
-import styled from 'styled-components';
-import { Colors } from 'theme/theme';
+import styled, { css } from 'styled-components';

-import { TableHeaderCellProps } from './TableHeaderCell';
+interface TitleProps {
+  isOrderable?: boolean;
+  isOrdered?: boolean;
+}

-export const TableHeaderCell = styled.th<TableHeaderCellProps>`
-  padding: 4px 0 4px 24px !important;
-  border-bottom-width: 1px !important;
-  vertical-align: middle !important;
+const isOrderableStyles = css`
+  cursor: pointer;

-  &.is-clickable {
-    cursor: pointer !important;
-    pointer-events: all !important;
-  }
-
-  &.has-text-link-dark span {
-    color: ${Colors.brand[50]} !important;
-  }
-
-  span {
-    font-family: Inter, sans-serif;
-    font-size: 12px;
-    font-style: normal;
-    font-weight: 400;
-    line-height: 16px;
-    letter-spacing: 0em;
-    text-align: left;
-    background: ${(props) => props.theme.thStyles.backgroundColor.normal};
-    color: ${(props) => props.theme.thStyles.color.normal};
-
-    &.preview {
-      margin-left: 8px;
-      font-size: 14px;
-      color: ${(props) => props.theme.thStyles.previewColor.normal};
-      cursor: pointer;
-    }
-
-    &.is-clickable {
-      cursor: pointer !important;
-      pointer-events: all !important;
-    }
+  &:hover {
+    color: ${(props) => props.theme.thStyles.color.hover};
  }
 `;
+
+export const Title = styled.span<TitleProps>`
+  font-family: Inter, sans-serif;
+  font-size: 12px;
+  font-style: normal;
+  font-weight: 400;
+  line-height: 16px;
+  letter-spacing: 0em;
+  text-align: left;
+  background: ${(props) => props.theme.thStyles.backgroundColor.normal};
+  color: ${(props) =>
+    props.isOrdered
+      ? props.theme.thStyles.color.active
+      : props.theme.thStyles.color.normal};
+  cursor: default;
+
+  ${(props) => props.isOrderable && isOrderableStyles}
+`;
+
+export const Preview = styled.span`
+  margin-left: 8px;
+  font-family: Inter, sans-serif;
+  font-style: normal;
+  font-weight: 400;
+  line-height: 16px;
+  letter-spacing: 0em;
+  text-align: left;
+  background: ${(props) => props.theme.thStyles.backgroundColor.normal};
+  font-size: 14px;
+  color: ${(props) => props.theme.thStyles.previewColor.normal};
+  cursor: pointer;
+`;
+
+export const TableHeaderCell = styled.th`
+  padding: 4px 0 4px 24px;
+  border-bottom-width: 1px;
+  vertical-align: middle;
+`;
--- a/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx
+++ b/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx
@ -1,7 +1,6 @@
 import React from 'react';
 import { TopicColumnsToSort } from 'generated-sources';
 import * as S from 'components/common/table/TableHeaderCell/TableHeaderCell.styled';
-import cx from 'classnames';

 export interface TableHeaderCellProps {
  title?: string;
@ -13,38 +12,57 @@ export interface TableHeaderCellProps {
 }

 const TableHeaderCell: React.FC<TableHeaderCellProps> = (props) => {
-  const { title, previewText, onPreview, orderBy, orderValue, handleOrderBy } =
-    props;
+  const {
+    title,
+    previewText,
+    onPreview,
+    orderBy,
+    orderValue,
+    handleOrderBy,
+    ...restProps
+  } = props;

+  const isOrdered = !!orderValue && orderValue === orderBy;
+  const isOrderable = !!(orderValue && handleOrderBy);
+
+  const handleOnClick = () => {
+    return orderValue && handleOrderBy && handleOrderBy(orderValue);
+  };
+  const handleOnKeyDown = (event: React.KeyboardEvent) => {
+    return (
+      event.code === 'Space' &&
+      orderValue &&
+      handleOrderBy &&
+      handleOrderBy(orderValue)
+    );
+  };
+  const orderableProps = isOrderable && {
+    isOrderable,
+    onClick: handleOnClick,
+    onKeyDown: handleOnKeyDown,
+    role: 'button',
+    tabIndex: 0,
+  };
  return (
-    <S.TableHeaderCell
-      className={cx(orderBy && orderBy === orderValue && 'has-text-link-dark')}
-      {...props}
-    >
-      <span className="title">{title}</span>
+    <S.TableHeaderCell {...restProps}>
+      <S.Title isOrdered={isOrdered} {...orderableProps}>
+        {title}
+        {isOrderable && (
+          <span title="Sort icon" className="icon is-small">
+            <i className="fas fa-sort" />
+          </span>
+        )}
+      </S.Title>
+
      {previewText && (
-        <span
-          className="preview"
+        <S.Preview
          onClick={onPreview}
          onKeyDown={onPreview}
          role="button"
          tabIndex={0}
        >
          {previewText}
-        </span>
-      )}
-      {orderValue && (
-        <span
-          className="icon is-small is-clickable"
-          onClick={() =>
-            orderValue && handleOrderBy && handleOrderBy(orderValue)
-          }
-          onKeyDown={() => handleOrderBy}
-          role="button"
-          tabIndex={0}
-        >
-          <i className="fas fa-sort" />
-        </span>
+        </S.Preview>
      )}
    </S.TableHeaderCell>
  );
--- a/kafka-ui-react-app/src/components/common/table/tests/TableHeaderCell.spec.tsx
+++ b/kafka-ui-react-app/src/components/common/table/tests/TableHeaderCell.spec.tsx
@ -1,17 +1,25 @@
 import React from 'react';
-import { StaticRouter } from 'react-router';
+import { screen, within } from '@testing-library/react';
+import { render } from 'lib/testHelpers';
 import TableHeaderCell, {
  TableHeaderCellProps,
 } from 'components/common/table/TableHeaderCell/TableHeaderCell';
-import { mountWithTheme } from 'lib/testHelpers';
 import { TopicColumnsToSort } from 'generated-sources';
+import theme from 'theme/theme';
+import userEvent from '@testing-library/user-event';

-const STUB_TITLE = 'stub test title';
-const STUB_PREVIEW_TEXT = 'stub preview text';
+const SPACE_KEY = ' ';
+
+const testTitle = 'test title';
+const testPreviewText = 'test preview text';
+const handleOrderBy = jest.fn();
+const onPreview = jest.fn();
+
+const sortIconTitle = 'Sort icon';

 describe('TableHeaderCell', () => {
-  const setupComponent = (props: TableHeaderCellProps) => (
-    <StaticRouter>
+  const setupComponent = (props: Partial<TableHeaderCellProps> = {}) =>
+    render(
      <table>
        <thead>
          <tr>
@ -19,49 +27,135 @@ describe('TableHeaderCell', () => {
          </tr>
        </thead>
      </table>
-    </StaticRouter>
-  );
+    );

  it('renders without props', () => {
-    const wrapper = mountWithTheme(setupComponent({}));
-    expect(wrapper.contains(<TableHeaderCell />)).toBeTruthy();
+    setupComponent();
+    expect(screen.getByRole('columnheader')).toBeInTheDocument();
  });

  it('renders with title & preview text', () => {
-    const wrapper = mountWithTheme(
-      setupComponent({
-        title: STUB_TITLE,
-        previewText: STUB_PREVIEW_TEXT,
-      })
-    );
+    setupComponent({
+      title: testTitle,
+      previewText: testPreviewText,
+    });

-    expect(wrapper.find('span.title').text()).toEqual(STUB_TITLE);
-    expect(wrapper.find('span.preview').text()).toEqual(STUB_PREVIEW_TEXT);
+    const columnheader = screen.getByRole('columnheader');
+    expect(within(columnheader).getByText(testTitle)).toBeInTheDocument();
+    expect(within(columnheader).getByText(testPreviewText)).toBeInTheDocument();
  });

-  it('renders with orderBy props', () => {
-    const wrapper = mountWithTheme(
-      setupComponent({
-        title: STUB_TITLE,
-        orderBy: TopicColumnsToSort.NAME,
-        orderValue: TopicColumnsToSort.NAME,
-      })
-    );
+  it('renders with orderable props', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+      orderValue: TopicColumnsToSort.NAME,
+      handleOrderBy,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByRole('button');
+    expect(title).toBeInTheDocument();
+    expect(title).toHaveTextContent(testTitle);
+    expect(within(title).getByTitle(sortIconTitle)).toBeInTheDocument();
+    expect(title).toHaveStyle(`color: ${theme.thStyles.color.active};`);
+    expect(title).toHaveStyle('cursor: pointer;');
+  });

-    expect(wrapper.find('span.title').text()).toEqual(STUB_TITLE);
-    expect(wrapper.exists('span.icon.is-small.is-clickable')).toBeTruthy();
-    expect(wrapper.exists('i.fas.fa-sort')).toBeTruthy();
+  it('renders click on title triggers handler', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+      orderValue: TopicColumnsToSort.NAME,
+      handleOrderBy,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByRole('button');
+    userEvent.click(title);
+    expect(handleOrderBy.mock.calls.length).toBe(1);
+  });
+
+  it('renders space on title triggers handler', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+      orderValue: TopicColumnsToSort.NAME,
+      handleOrderBy,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByRole('button');
+    userEvent.type(title, SPACE_KEY);
+    // userEvent.type clicks and only then presses space
+    expect(handleOrderBy.mock.calls.length).toBe(2);
+  });
+
+  it('click on preview triggers handler', () => {
+    setupComponent({
+      title: testTitle,
+      previewText: testPreviewText,
+      onPreview,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const preview = within(columnheader).getByRole('button');
+    userEvent.click(preview);
+    expect(onPreview.mock.calls.length).toBe(1);
+  });
+
+  it('click on preview triggers handler', () => {
+    setupComponent({
+      title: testTitle,
+      previewText: testPreviewText,
+      onPreview,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const preview = within(columnheader).getByRole('button');
+    userEvent.type(preview, SPACE_KEY);
+    // userEvent.type clicks and only then presses space
+    expect(onPreview.mock.calls.length).toBe(2);
+  });
+
+  it('renders without sort indication', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+    });
+
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByText(testTitle);
+    expect(within(title).queryByTitle(sortIconTitle)).not.toBeInTheDocument();
+    expect(title).toHaveStyle('cursor: default;');
+  });
+
+  it('renders with hightlighted title when orderBy and orderValue are equal', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+      orderValue: TopicColumnsToSort.NAME,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByText(testTitle);
+    expect(title).toHaveStyle(`color: ${theme.thStyles.color.active};`);
+  });
+
+  it('renders without hightlighted title when orderBy and orderValue are not equal', () => {
+    setupComponent({
+      title: testTitle,
+      orderBy: TopicColumnsToSort.NAME,
+      orderValue: TopicColumnsToSort.OUT_OF_SYNC_REPLICAS,
+    });
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByText(testTitle);
+    expect(title).toHaveStyle(`color: ${theme.thStyles.color.normal}`);
  });

  it('renders with default (primary) theme', () => {
-    const wrapper = mountWithTheme(
-      setupComponent({
-        title: STUB_TITLE,
-      })
-    );
+    setupComponent({
+      title: testTitle,
+    });

-    const domNode = wrapper.find('span').at(0).getDOMNode();
-    const background = getComputedStyle(domNode).getPropertyValue('background');
-    expect(background).toBe('rgb(255, 255, 255)');
+    const columnheader = screen.getByRole('columnheader');
+    const title = within(columnheader).getByText(testTitle);
+    expect(title).toHaveStyle(
+      `background: ${theme.thStyles.backgroundColor.normal};`
+    );
  });
 });
--- a/kafka-ui-react-app/src/redux/reducers/topics/reducer.ts
+++ b/kafka-ui-react-app/src/redux/reducers/topics/reducer.ts
@ -2,13 +2,14 @@ import { Action, TopicsState } from 'redux/interfaces';
 import { getType } from 'typesafe-actions';
 import * as actions from 'redux/actions';
 import * as _ from 'lodash';
+import { TopicColumnsToSort } from 'generated-sources';

 export const initialState: TopicsState = {
  byName: {},
  allNames: [],
  totalPages: 1,
  search: '',
-  orderBy: null,
+  orderBy: TopicColumnsToSort.NAME,
  consumerGroups: [],
 };

--- a/kafka-ui-react-app/src/theme/theme.ts
+++ b/kafka-ui-react-app/src/theme/theme.ts
@ -133,6 +133,8 @@ const theme = {
    },
    color: {
      normal: Colors.neutral[50],
+      hover: Colors.brand[50],
+      active: Colors.brand[50],
    },
    previewColor: {
      normal: Colors.brand[50],