From cdc929add70a9e5e0987a089b6c436079cae32f1 Mon Sep 17 00:00:00 2001 From: Damir Abdulganiev Date: Wed, 19 Jan 2022 12:43:00 +0300 Subject: [PATCH] [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 * Update kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx Co-authored-by: Oleg Shur * 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 Co-authored-by: Damir Abdulganiev Co-authored-by: Oleg Shur --- .../__snapshots__/Tasks.spec.tsx.snap | 92 +++------- .../__snapshots__/ListItem.spec.tsx.snap | 2 + .../__snapshots__/Details.spec.tsx.snap | 2 + .../TableHeaderCell/TableHeaderCell.styled.ts | 85 +++++---- .../table/TableHeaderCell/TableHeaderCell.tsx | 66 ++++--- .../table/__tests__/TableHeaderCell.spec.tsx | 168 ++++++++++++++---- .../src/redux/reducers/topics/reducer.ts | 3 +- kafka-ui-react-app/src/theme/theme.ts | 2 + 8 files changed, 250 insertions(+), 170 deletions(-) diff --git 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 index 34f655c637..7f14f530d1 100644 --- 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; } @@ -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; }
ID Worker State Trace @@ -109,7 +85,7 @@ exports[`Tasks view matches snapshot 1`] = ` className="c1" >
diff --git 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 index 68d4d62415..6cd4d47769 100644 --- 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 { diff --git 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 index b6e1ccbaf2..f40d195488 100644 --- 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 { diff --git 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 index 6d4fbf847c..6e4d44d65f 100644 --- 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` - 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` + 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; +`; diff --git a/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx b/kafka-ui-react-app/src/components/common/table/TableHeaderCell/TableHeaderCell.tsx index 66a75835ce..bf90430e00 100644 --- 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 = (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 ( - - {title} + + + {title} + {isOrderable && ( + + + + )} + + {previewText && ( - {previewText} - - )} - {orderValue && ( - - orderValue && handleOrderBy && handleOrderBy(orderValue) - } - onKeyDown={() => handleOrderBy} - role="button" - tabIndex={0} - > - - + )} ); diff --git 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 index 7b5fd4e2ab..7742f67b70 100644 --- 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) => ( - + const setupComponent = (props: Partial = {}) => + render(
ID Worker State Trace @@ -294,7 +246,7 @@ exports[`Tasks view matches snapshot when no tasks 1`] = ` className="c1" >
@@ -19,49 +27,135 @@ describe('TableHeaderCell', () => {
- - ); + ); it('renders without props', () => { - const wrapper = mountWithTheme(setupComponent({})); - expect(wrapper.contains()).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};` + ); }); }); diff --git a/kafka-ui-react-app/src/redux/reducers/topics/reducer.ts b/kafka-ui-react-app/src/redux/reducers/topics/reducer.ts index 485793cd21..5e4426d77d 100644 --- 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: [], }; diff --git a/kafka-ui-react-app/src/theme/theme.ts b/kafka-ui-react-app/src/theme/theme.ts index c1103242c1..a8cc8523e3 100644 --- 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],