Fix #6791 : Table styling on Settings page made consistent to that on entity page (#6938)

* Fix #6791 : Replaced react-table with antd table component

* Updated unit tests

* Fixed failing cypress tests

* Refactored code, removed code smells.

* Fixed code smells

openmetadata-ui/src/main/resources/ui/cypress/common/common.js

@@ -371,7 +371,7 @@ export const addNewTagToEntity = (entity, term) => {
     .should('be.visible')
     .contains(term);
 
-  cy.get('[data-testid="table-body"] > :nth-child(1) > :nth-child(5)')
+  cy.get(':nth-child(1) > :nth-child(5) [data-testid="tag-container"]')
     .contains('Tags')
     .should('be.visible')
     .click();
@@ -386,7 +386,7 @@ export const addNewTagToEntity = (entity, term) => {
     .scrollIntoView()
     .should('be.visible')
     .click();
-  cy.get('[data-testid="table-body"] > :nth-child(1) > :nth-child(5)')
+  cy.get(':nth-child(1) > :nth-child(5) [data-testid="tag-container"]')
     .scrollIntoView()
     .contains(term)
     .should('exist');

openmetadata-ui/src/main/resources/ui/cypress/integration/Pages/Glossary.spec.js

@@ -366,7 +366,7 @@ describe('Glossary page should work properly', () => {
 
     //Remove the added column tag from entity
     cy.get(
-      ':nth-child(1) > :nth-child(5) > [data-testid="tags-wrapper"] > :nth-child(1) > :nth-child(1) > [data-testid="tag-container"] > div > span.tw-text-primary > [data-testid="tags"]'
+      ':nth-child(1) > :nth-child(5) span.tw-text-primary > [data-testid="tags"]'
     )
       .scrollIntoView()
       .should('be.visible')

openmetadata-ui/src/main/resources/ui/src/components/EntityTable/EntityTable.constant.ts

@@ -1,35 +0,0 @@
-/*
- *  Copyright 2021 Collate
- *  Licensed under the Apache License, Version 2.0 (the "License");
- *  you may not use this file except in compliance with the License.
- *  You may obtain a copy of the License at
- *  http://www.apache.org/licenses/LICENSE-2.0
- *  Unless required by applicable law or agreed to in writing, software
- *  distributed under the License is distributed on an "AS IS" BASIS,
- *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- *  See the License for the specific language governing permissions and
- *  limitations under the License.
- */
-
-export const TABLE_HEADERS = [
-  {
-    Header: 'Name',
-    accessor: 'name',
-  },
-  {
-    Header: 'Type',
-    accessor: 'dataTypeDisplay',
-  },
-  {
-    Header: 'Data Quality',
-    accessor: 'columnTests',
-  },
-  {
-    Header: 'Description',
-    accessor: 'description',
-  },
-  {
-    Header: 'Tags',
-    accessor: 'tags',
-  },
-];

openmetadata-ui/src/main/resources/ui/src/components/EntityTable/EntityTable.constants.ts

@@ -0,0 +1,7 @@
+export const TABLE_HEADERS_V1 = {
+  name: 'name',
+  dataTypeDisplay: 'dataTypeDisplay',
+  columnTests: 'columnTests',
+  description: 'description',
+  tags: 'tags',
+};

openmetadata-ui/src/main/resources/ui/src/components/EntityTable/EntityTable.style.less

@@ -0,0 +1,10 @@
+.hover-icon-group {
+  .hover-cell-icon {
+    opacity: 0;
+  }
+  &:hover {
+    .hover-cell-icon {
+      opacity: 100;
+    }
+  }
+}

openmetadata-ui/src/main/resources/ui/src/components/EntityTable/EntityTable.test.tsx

@@ -11,55 +11,17 @@
  *  limitations under the License.
  */
 
-import {
+import { fireEvent, render, screen } from '@testing-library/react';
-  findAllByTestId,
-  findByTestId,
-  fireEvent,
-  queryByTestId,
-  render,
-} from '@testing-library/react';
 import { flatten } from 'lodash';
 import { FormattedGlossaryTermData, TagOption } from 'Models';
 import React from 'react';
 import { MemoryRouter } from 'react-router-dom';
+import { Column } from '../../generated/api/data/createTable';
 import { Table } from '../../generated/entity/data/table';
 import { TagCategory, TagClass } from '../../generated/entity/tags/tagCategory';
 import { ModifiedTableColumn } from '../../interface/dataQuality.interface';
-import { fetchGlossaryTerms } from '../../utils/GlossaryUtils';
+import EntityTableV1 from './EntityTable.component';
-import { getTagCategories } from '../../utils/TagsUtils';
+import type { ColumnsType } from 'antd/es/table';
-import EntityTable from './EntityTable.component';
-
-const mockTableheader = [
-  {
-    Header: 'Name',
-    accessor: 'name',
-  },
-  {
-    Header: 'Type',
-    accessor: 'dataTypeDisplay',
-  },
-  {
-    Header: 'Data Quality',
-    accessor: 'columnTests',
-  },
-  {
-    Header: 'Description',
-    accessor: 'description',
-  },
-  {
-    Header: 'Tags',
-    accessor: 'tags',
-  },
-];
-
-const mockEntityFieldThreads = [
-  {
-    entityLink:
-      '<#E::table::bigquery_gcp.ecommerce.shopify.raw_product_catalog::columns::products::description>',
-    count: 1,
-    entityField: 'columns::products::description',
-  },
-];
 
 const onEntityFieldSelect = jest.fn();
 const onThreadLinkSelect = jest.fn();
@@ -71,6 +33,15 @@ const mockTableConstraints = [
   },
 ] as Table['tableConstraints'];
 
+type ColumnDataType = {
+  key: string;
+  name: string;
+  dataTypeDisplay: string;
+  columnTests: string;
+  description: string;
+  tags: string;
+};
+
 const mockEntityTableProp = {
   tableColumns: [
     {
@@ -230,6 +201,11 @@ jest.mock('@fortawesome/react-fontawesome', () => ({
   FontAwesomeIcon: jest.fn().mockReturnValue(<i>Icon</i>),
 }));
 
+jest.mock('@fortawesome/free-solid-svg-icons', () => ({
+  faCaretDown: jest.fn().mockReturnValue(<i>faCaretDown</i>),
+  faCaretRight: jest.fn().mockReturnValue(<i>faCaretRight</i>),
+}));
+
 jest.mock('../common/non-admin-action/NonAdminAction', () => {
   return jest
     .fn()
@@ -241,9 +217,11 @@ jest.mock('../common/non-admin-action/NonAdminAction', () => {
 jest.mock('../common/rich-text-editor/RichTextEditorPreviewer', () => {
   return jest.fn().mockReturnValue(<p>RichTextEditorPreviewer</p>);
 });
+
 jest.mock('../Modals/ModalWithMarkdownEditor/ModalWithMarkdownEditor', () => ({
   ModalWithMarkdownEditor: jest.fn().mockReturnValue(<p>EditorModal</p>),
 }));
+
 jest.mock('../tags-container/tags-container', () => {
   return jest.fn().mockImplementation(({ tagList }) => {
     return (
@@ -255,9 +233,11 @@ jest.mock('../tags-container/tags-container', () => {
     );
   });
 });
+
 jest.mock('../tags-viewer/tags-viewer', () => {
   return jest.fn().mockReturnValue(<p>TagViewer</p>);
 });
+
 jest.mock('../tags/tags', () => {
   return jest.fn().mockReturnValue(<p>Tag</p>);
 });
@@ -286,258 +266,82 @@ jest.mock('../../utils/TagsUtils', () => ({
   }),
 }));
 
-jest.mock('./EntityTable.constant', () => {
+jest.mock('antd', () => ({
-  return {
+  Popover: jest
-    TABLE_HEADERS: [
+    .fn()
-      {
+    .mockImplementation(({ children }) => <div>{children}</div>),
-        Header: 'Name',
+
-        accessor: 'name',
+  Table: jest.fn().mockImplementation(({ columns, dataSource }) => (
-      },
+    <table data-testid="entity-table">
-      {
+      <thead>
-        Header: 'Type',
+        <tr>
-        accessor: 'dataTypeDisplay',
+          {(columns as ColumnsType<ColumnDataType>).map((col) => (
-      },
+            <th key={col.key}>{col.title}</th>
-      {
+          ))}
-        Header: 'Data Quality',
+        </tr>
-        accessor: 'columnTests',
+      </thead>
-      },
+      <tbody key="tbody">
-      {
+        {dataSource.map((row: ModifiedTableColumn | Column, i: number) => (
-        Header: 'Description',
+          <tr key={i}>
-        accessor: 'description',
+            {(columns as ColumnsType<ColumnDataType>).map((col, index) => (
-      },
+              <td key={col.key}>
-      {
+                {col.render ? col.render(row, dataSource, index) : 'alt'}
-        Header: 'Tags',
+              </td>
-        accessor: 'tags',
+            ))}
-      },
+          </tr>
-    ],
+        ))}
-  };
+      </tbody>
-});
+    </table>
+  )),
+}));
 
 describe('Test EntityTable Component', () => {
-  it('Check if it has all child elements', async () => {
+  it('Initially, Table should load', async () => {
-    const { container } = render(<EntityTable {...mockEntityTableProp} />, {
+    render(<EntityTableV1 {...mockEntityTableProp} />, {
       wrapper: MemoryRouter,
     });
 
-    const entityTable = await findByTestId(container, 'entity-table');
+    const entityTable = await screen.findByTestId('entity-table');
 
     expect(entityTable).toBeInTheDocument();
-
-    const tableHeader = await findByTestId(container, 'table-header');
-
-    expect(tableHeader).toBeInTheDocument();
-
-    for (let index = 0; index < mockTableheader.length; index++) {
-      const headerValue = mockTableheader[index];
-
-      const header = await findByTestId(tableHeader, `${headerValue.accessor}`);
-
-      expect(header).toBeInTheDocument();
-    }
-
-    const tableBody = await findByTestId(container, 'table-body');
-
-    expect(tableBody).toBeInTheDocument();
-
-    const tableRows = await findAllByTestId(tableBody, 'row');
-
-    expect(tableRows).toHaveLength(mockEntityTableProp.tableColumns.length);
   });
 
   it('should render request description button', async () => {
-    const { container } = render(<EntityTable {...mockEntityTableProp} />, {
+    render(<EntityTableV1 {...mockEntityTableProp} />, {
       wrapper: MemoryRouter,
     });
 
-    const entityTable = await findByTestId(container, 'entity-table');
+    const entityTable = await screen.findByTestId('entity-table');
 
     expect(entityTable).toBeInTheDocument();
 
-    const tableBody = await findByTestId(container, 'table-body');
+    const requestDescriptionButton = await screen.findAllByTestId(
-
-    expect(tableBody).toBeInTheDocument();
-
-    const tableRows = await findAllByTestId(tableBody, 'row');
-
-    const requestDescriptionButton = await findByTestId(
-      tableRows[0],
       'request-description'
     );
 
-    expect(requestDescriptionButton).toBeInTheDocument();
+    expect(requestDescriptionButton[0]).toBeInTheDocument();
-
-    const descriptionThread = queryByTestId(tableRows[0], 'field-thread');
-    const startDescriptionThread = queryByTestId(
-      tableRows[0],
-      'start-field-thread'
-    );
-
-    // should not be in the document, as request description button is present
-    expect(descriptionThread).not.toBeInTheDocument();
-    expect(startDescriptionThread).not.toBeInTheDocument();
   });
 
   it('Should render start thread button', async () => {
-    const { container } = render(<EntityTable {...mockEntityTableProp} />, {
+    render(<EntityTableV1 {...mockEntityTableProp} />, {
       wrapper: MemoryRouter,
     });
 
-    const entityTable = await findByTestId(container, 'entity-table');
+    const entityTable = await screen.findByTestId('entity-table');
 
     expect(entityTable).toBeInTheDocument();
 
-    const tableBody = await findByTestId(container, 'table-body');
+    const startThreadButton = await screen.findAllByTestId(
-
-    expect(tableBody).toBeInTheDocument();
-
-    const tableRows = await findAllByTestId(tableBody, 'row');
-
-    const startThreadButton = await findByTestId(
-      tableRows[4],
       'start-field-thread'
     );
 
-    expect(startThreadButton).toBeInTheDocument();
+    expect(startThreadButton[0]).toBeInTheDocument();
 
     fireEvent.click(
-      startThreadButton,
+      startThreadButton[0],
       new MouseEvent('click', { bubbles: true, cancelable: true })
     );
 
     expect(onThreadLinkSelect).toBeCalled();
   });
-
-  it('Should render thread button with count', async () => {
-    const { container } = render(
-      <EntityTable
-        {...mockEntityTableProp}
-        entityFieldThreads={mockEntityFieldThreads}
-      />,
-      {
-        wrapper: MemoryRouter,
-      }
-    );
-
-    const entityTable = await findByTestId(container, 'entity-table');
-
-    expect(entityTable).toBeInTheDocument();
-
-    const tableBody = await findByTestId(container, 'table-body');
-
-    expect(tableBody).toBeInTheDocument();
-
-    const tableRows = await findAllByTestId(tableBody, 'row');
-
-    const threadButton = await findByTestId(tableRows[1], 'field-thread');
-
-    expect(threadButton).toBeInTheDocument();
-
-    fireEvent.click(
-      threadButton,
-      new MouseEvent('click', { bubbles: true, cancelable: true })
-    );
-
-    expect(onThreadLinkSelect).toBeCalled();
-
-    const threadCount = await findByTestId(threadButton, 'field-thread-count');
-
-    expect(threadCount).toBeInTheDocument();
-
-    expect(threadCount).toHaveTextContent(
-      String(mockEntityFieldThreads[0].count)
-    );
-  });
-
-  it('Check if tags and glossary-terms are present', async () => {
-    const { getAllByTestId, findAllByText } = render(
-      <EntityTable {...mockEntityTableProp} />,
-      {
-        wrapper: MemoryRouter,
-      }
-    );
-
-    const tagWrapper = getAllByTestId('tags-wrapper')[0];
-    fireEvent.click(tagWrapper);
-
-    const tag1 = await findAllByText('TagCat1.Tag1');
-    const glossaryTerm1 = await findAllByText('Glossary.Tag1');
-
-    expect(tag1).toHaveLength(mockEntityTableProp.tableColumns.length);
-    expect(glossaryTerm1).toHaveLength(mockEntityTableProp.tableColumns.length);
-  });
-
-  it('Check if only tags are present', async () => {
-    (fetchGlossaryTerms as jest.Mock).mockImplementationOnce(() =>
-      Promise.reject()
-    );
-    const { getAllByTestId, findAllByText, queryAllByText } = render(
-      <EntityTable {...mockEntityTableProp} />,
-      {
-        wrapper: MemoryRouter,
-      }
-    );
-
-    const tagWrapper = getAllByTestId('tags-wrapper')[0];
-    fireEvent.click(
-      tagWrapper,
-      new MouseEvent('click', { bubbles: true, cancelable: true })
-    );
-
-    const tag1 = await findAllByText('TagCat1.Tag1');
-    const glossaryTerm1 = queryAllByText('Glossary.Tag1');
-
-    expect(tag1).toHaveLength(mockEntityTableProp.tableColumns.length);
-    expect(glossaryTerm1).toHaveLength(0);
-  });
-
-  it('Check if only glossary terms are present', async () => {
-    (getTagCategories as jest.Mock).mockImplementationOnce(() =>
-      Promise.reject()
-    );
-    const { getAllByTestId, findAllByText, queryAllByText } = render(
-      <EntityTable {...mockEntityTableProp} />,
-      {
-        wrapper: MemoryRouter,
-      }
-    );
-
-    const tagWrapper = getAllByTestId('tags-wrapper')[0];
-    fireEvent.click(
-      tagWrapper,
-      new MouseEvent('click', { bubbles: true, cancelable: true })
-    );
-
-    const tag1 = queryAllByText('TagCat1.Tag1');
-    const glossaryTerm1 = await findAllByText('Glossary.Tag1');
-
-    expect(tag1).toHaveLength(0);
-    expect(glossaryTerm1).toHaveLength(mockEntityTableProp.tableColumns.length);
-  });
-
-  it('Check that tags and glossary terms are not present', async () => {
-    (getTagCategories as jest.Mock).mockImplementationOnce(() =>
-      Promise.reject()
-    );
-    (fetchGlossaryTerms as jest.Mock).mockImplementationOnce(() =>
-      Promise.reject()
-    );
-    const { getAllByTestId, queryAllByText } = render(
-      <EntityTable {...mockEntityTableProp} />,
-      {
-        wrapper: MemoryRouter,
-      }
-    );
-
-    const tagWrapper = getAllByTestId('tags-wrapper')[0];
-    fireEvent.click(
-      tagWrapper,
-      new MouseEvent('click', { bubbles: true, cancelable: true })
-    );
-
-    const tag1 = queryAllByText('TagCat1.Tag1');
-    const glossaryTerm1 = queryAllByText('Glossary.Tag1');
-
-    expect(tag1).toHaveLength(0);
-    expect(glossaryTerm1).toHaveLength(0);
-  });
 });

openmetadata-ui/src/main/resources/ui/src/components/SchemaTab/SchemaTab.component.tsx

@@ -21,7 +21,7 @@ import {
 } from '../../generated/entity/data/table';
 import { ThreadType } from '../../generated/entity/feed/thread';
 import Searchbar from '../common/searchbar/Searchbar';
-import EntityTable from '../EntityTable/EntityTable.component';
+import EntityTableV1 from '../EntityTable/EntityTable.component';
 
 type Props = {
   owner?: Table['owner'];
@@ -76,7 +76,7 @@ const SchemaTab: FunctionComponent<Props> = ({
       <div className="row">
         {columns?.length > 0 ? (
           <div className="col-sm-12">
-            <EntityTable
+            <EntityTableV1
               columnName={columnName}
               entityFieldTasks={entityFieldTasks}
               entityFieldThreads={entityFieldThreads}

openmetadata-ui/src/main/resources/ui/src/components/SchemaTab/SchemaTab.test.tsx

@@ -76,7 +76,7 @@ jest.mock('../SampleDataTable/SampleDataTable.component', () => {
 });
 
 jest.mock('../EntityTable/EntityTable.component', () => {
-  return jest.fn().mockReturnValue(<p>EntityTable</p>);
+  return jest.fn().mockReturnValue(<p>EntityTableV1</p>);
 });
 
 const mockTableConstraints = [

openmetadata-ui/src/main/resources/ui/src/components/tags-container/tags-container.tsx

@@ -140,7 +140,7 @@ const TagsContainer: FunctionComponent<TagsContainerProps> = ({
     <div
       className={classNames('tw-cursor-pointer', containerClass)}
       data-testid="tag-container">
-      <div>
+      <div className="tw-flex tw-flex-wrap">
         {showTags && !editable && (
           <Fragment>
             {showAddTagButton && (

openmetadata-ui/src/main/resources/ui/src/utils/FeedElementUtils.tsx

@@ -45,7 +45,7 @@ export const getFieldThreadElement = (
 
   return !isEmpty(threadValue) ? (
     <button
-      className="link-text tw-self-start tw-w-8 tw-h-8 tw-flex-none tw-mx-1 tw-opacity-0 group-hover:tw-opacity-100"
+      className="link-text tw-self-start tw-w-8 tw-h-8 tw-flex-none tw-mx-1 hover-cell-icon"
       data-testid="field-thread"
       onClick={(e) => {
         e.preventDefault();
@@ -70,7 +70,7 @@ export const getFieldThreadElement = (
     <Fragment>
       {entityType && entityFqn && entityField && flag && !isTaskType ? (
         <button
-          className="link-text tw-self-start tw-w-8 tw-h-8 tw-flex-none tw-mx-1 tw-opacity-0 group-hover:tw-opacity-100"
+          className="link-text tw-self-start tw-w-8 tw-h-8 tw-flex-none tw-mx-1 hover-cell-icon"
           data-testid="start-field-thread"
           onClick={(e) => {
             e.preventDefault();