Improve InlineSearch UI (#355)

* Update inline search UI according to design QA

* Update tests

* Address some comments

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/SearchItemList/SearchItem/index.tsx

 import * as React from 'react';
+import { connect } from 'react-redux'
 
 import { logClick } from 'ducks/utilMethods';
 import { ResourceType } from 'interfaces';
 
-export interface SearchItemProps {
+import LoadingSpinner from 'components/common/LoadingSpinner';
+
+import { GlobalState } from 'ducks/rootReducer'
+
+import {
+  SEARCH_ITEM_NO_RESULTS
+} from 'components/common/SearchBar/InlineSearchResults/constants';
+
+export interface StateFromProps {
+  isLoading: boolean;
+  hasResults: boolean;
+}
+
+export interface OwnProps {
   listItemText: string;
   onItemSelect: (resourceType: ResourceType, updateUrl: boolean) => void;
   searchTerm: string;
   resourceType: ResourceType;
 }
 
-class SearchItem extends React.Component<SearchItemProps, {}> {
+export type SearchItemProps = StateFromProps & OwnProps;
+
+export class SearchItem extends React.Component<SearchItemProps, {}> {
   constructor(props) {
     super(props);
   }
@@ -20,6 +36,20 @@ class SearchItem extends React.Component<SearchItemProps, {}> {
     this.props.onItemSelect(this.props.resourceType, true);
   }
 
+  renderIndicator = () => {
+    if (this.props.isLoading) {
+      return (<LoadingSpinner/>)
+    }
+    if (!this.props.hasResults) {
+      return (
+        <div className="search-item-indicator body-placeholder">
+          { SEARCH_ITEM_NO_RESULTS }
+        </div>
+      )
+    }
+    return null;
+  }
+
   render = () => {
     const { searchTerm, listItemText, resourceType } = this.props;
     return (
@@ -35,10 +65,29 @@ class SearchItem extends React.Component<SearchItemProps, {}> {
             <div className="search-term">{`${searchTerm}\u00a0`}</div>
             <div className="search-item-text">{listItemText}</div>
           </div>
+          { this.renderIndicator() }
         </a>
       </li>
     );
   }
 };
 
-export default SearchItem;
+export const mapStateToProps = (state: GlobalState, ownProps: OwnProps) => {
+  const { isLoading, tables, users } = state.search.inlineResults;
+  let hasResults = false;
+  switch (ownProps.resourceType) {
+    case ResourceType.table:
+      hasResults = tables.results.length > 0;
+      break;
+    case ResourceType.user:
+      hasResults = users.results.length > 0;
+    default:
+      break;
+  }
+  return {
+    isLoading,
+    hasResults
+  };
+};
+
+export default connect<{}, {}, OwnProps>(mapStateToProps)(SearchItem);

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/SearchItemList/SearchItem/tests/index.spec.tsx

@@ -2,10 +2,19 @@ import * as React from 'react';
 
 import { shallow } from 'enzyme';
 
-import SearchItem, { SearchItemProps } from '../';
+import LoadingSpinner from 'components/common/LoadingSpinner';
+import { SearchItem, SearchItemProps, mapStateToProps } from '../';
+
+import {
+  SEARCH_ITEM_NO_RESULTS
+} from 'components/common/SearchBar/InlineSearchResults/constants';
 
 import { ResourceType } from 'interfaces';
 
+import { GlobalState } from 'ducks/rootReducer'
+import globalState from 'fixtures/globalState';
+import { allResourcesExample, isLoadingExample, noResultsExample } from 'fixtures/search/inlineResults';
+
 import { logClick } from 'ducks/utilMethods';
 jest.mock('ducks/utilMethods', () => (
   {
@@ -20,6 +29,8 @@ describe('SearchItem', () => {
       onItemSelect: jest.fn(),
       searchTerm: 'test search',
       resourceType: ResourceType.table,
+      isLoading: false,
+      hasResults: true,
       ...propOverrides
     };
     const wrapper = shallow<SearchItem>(<SearchItem {...props} />);
@@ -35,13 +46,37 @@ describe('SearchItem', () => {
     })
   });
 
+  describe('renderIndicator', () => {
+    it('renders LoadingSpinner if props.isLoading', () => {
+      const { props, wrapper } = setup({ isLoading: true });
+      const content = shallow(<div>{wrapper.instance().renderIndicator()}</div>);
+      expect(content.find(LoadingSpinner).exists()).toBe(true);
+    });
+
+    it('renders correct text if !props.hasResults', () => {
+      const { props, wrapper } = setup({ hasResults: false });
+      const content = shallow(wrapper.instance().renderIndicator());
+      expect(content.text()).toBe(SEARCH_ITEM_NO_RESULTS);
+    });
+
+    it('renders nothing if !props.Loading and props.hasResults', () => {
+      const { props, wrapper } = setup({ isLoading: false, hasResults: true});
+      expect(wrapper.instance().renderIndicator()).toBe(null);
+    });
+  });
+
   describe('render', () => {
     let props;
     let wrapper;
+    let renderIndicatorSpy;
+    let mockContent;
     beforeAll(() => {
       const setUpResult = setup();
       props = setUpResult.props;
       wrapper = setUpResult.wrapper;
+      mockContent = (<div>Hello</div>);
+      renderIndicatorSpy = jest.spyOn(wrapper.instance(), 'renderIndicator').mockImplementation(() => mockContent);
+      wrapper.instance().forceUpdate();
     });
 
     describe('renders list item link', () => {
@@ -64,8 +99,81 @@ describe('SearchItem', () => {
         });
 
         it('renders correct text', () => {
-          expect(listItemLink.text()).toEqual(`${props.searchTerm}\u00a0${props.listItemText}`);
+          expect(listItemLink.find('.search-item-info').text()).toEqual(`${props.searchTerm}\u00a0${props.listItemText}`);
         });
+
+        it('renders results of renderIndicator', () => {
+          expect(listItemLink.children().at(2).html()).toEqual("<div>Hello</div>");
+        });
+      });
+    });
+
+    it('calls renderIndicator', () => {
+      renderIndicatorSpy.mockClear();
+      wrapper.instance().render();
+      expect(renderIndicatorSpy).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('mapStateToProps', () => {
+    let result;
+    let ownProps;
+    const mockLoadingState: GlobalState = {
+      ...globalState,
+      search: {
+        ...globalState.search,
+        inlineResults: isLoadingExample,
+      }
+    };
+    const mockAllResultsState: GlobalState = {
+      ...globalState,
+      search: {
+        ...globalState.search,
+        // @ts-ignore: https://github.com/microsoft/TypeScript/issues/10570
+        inlineResults: allResourcesExample,
+      }
+    };
+    const mockNoResultsState: GlobalState = {
+      ...globalState,
+      search: {
+        ...globalState.search,
+        inlineResults: noResultsExample,
+      }
+    };
+
+    it('sets isLoading on the props', () => {
+      ownProps = setup().props;
+      result = mapStateToProps(mockLoadingState, ownProps);
+      expect(result.isLoading).toEqual(true);
+    });
+
+    describe('ownProps.resourceType is ResourceType.table', () => {
+      beforeAll(() => {
+        ownProps = setup({resourceType: ResourceType.table}).props;
+      });
+      it('sets hasResults to true if there are table results', () => {
+        result = mapStateToProps(mockAllResultsState, ownProps);
+        expect(result.hasResults).toEqual(true);
+      });
+
+      it('sets hasResults to false if there are no table results', () => {
+        result = mapStateToProps(mockNoResultsState, ownProps);
+        expect(result.hasResults).toEqual(false);
+      });
+    });
+
+    describe('ownProps.resourceType is ResourceType.user', () => {
+      beforeAll(() => {
+        ownProps = setup({resourceType: ResourceType.user}).props;
+      })
+      it('sets hasResults to true if there are user results', () => {
+        result = mapStateToProps(mockAllResultsState, ownProps);
+        expect(result.hasResults).toEqual(true);
+      });
+
+      it('sets hasResults to false if there are no user results', () => {
+        result = mapStateToProps(mockNoResultsState, ownProps);
+        expect(result.hasResults).toEqual(false);
       });
     });
   });

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/SearchItemList/tests/index.spec.tsx

@@ -12,6 +12,14 @@ import * as CONSTANTS from '../../constants';
 jest.mock('config/config-utils', () => ({ indexUsersEnabled: jest.fn() }));
 import { indexUsersEnabled } from 'config/config-utils';
 
+jest.mock("react-redux", () => {
+  return {
+    connect: (mapStateToProps, mapDispatchToProps) => (
+      SearchItem
+    ) => SearchItem
+  };
+});
+
 describe('SearchItemList', () => {
   const setup = (propOverrides?: Partial<SearchItemListProps>) => {
     const props: SearchItemListProps = {

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/constants.ts

@@ -11,3 +11,5 @@ export const USER_ICON_CLASS = "icon-users";
 
 export const RESULT_LIST_FOOTER_PREFIX = "See all";
 export const RESULT_LIST_FOOTER_SUFFIX = "results";
+
+export const SEARCH_ITEM_NO_RESULTS = "No results found";

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/index.tsx

 import * as React from 'react';
 import { connect } from 'react-redux'
 
-import LoadingSpinner from 'components/common/LoadingSpinner';
 import SearchItemList from './SearchItemList';
 import ResultItemList from './ResultItemList';
 
@@ -153,13 +152,17 @@ export class InlineSearchResults extends React.Component<InlineSearchResultsProp
   };
 
   renderResultsByResource = (resourceType: ResourceType) => {
+    const suggestedResults = this.getSuggestedResultsForResource(resourceType);
+    if (suggestedResults.length === 0) {
+      return null;
+    }
     return (
       <div className="inline-results-section">
         <ResultItemList
           onItemSelect={this.props.onItemSelect}
           resourceType={resourceType}
           searchTerm={this.props.searchTerm}
-          suggestedResults={this.getSuggestedResultsForResource(resourceType)}
+          suggestedResults={suggestedResults}
           totalResults={this.getTotalResultsForResource(resourceType)}
           title={this.getTitleForResource(resourceType)}
         />
@@ -169,11 +172,7 @@ export class InlineSearchResults extends React.Component<InlineSearchResultsProp
 
   renderResults = () => {
     if (this.props.isLoading) {
-      return (
-        <div className="inline-results-section">
-          <LoadingSpinner/>
-        </div>
-      );
+      return null;
     }
     return (
       <>
@@ -190,7 +189,7 @@ export class InlineSearchResults extends React.Component<InlineSearchResultsProp
     const { className = '', onItemSelect, searchTerm } = this.props;
     return (
       <div id="inline-results" className={`inline-results ${className}`}>
-        <div className="inline-results-section">
+        <div className="inline-results-section search-item-section">
           <SearchItemList
             onItemSelect={onItemSelect}
             searchTerm={searchTerm}

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/styles.scss

@@ -17,6 +17,7 @@
 
     .result-item-link {
       padding-left: 20px !important;
+      padding-right: 20px !important;
     }
 
     .section-footer.title-3,
@@ -25,17 +26,19 @@
     }
   }
 
-  .inline-results-section {
-    padding-top: 8px;
-    padding-bottom: 8px;
+  .search-item-section {
+    padding-top: $spacer-1;
+    padding-bottom: $spacer-1;
+  }
 
+  .inline-results-section {
     &:not(:last-of-type) {
       border-bottom: 1px solid $stroke;
     }
 
     .section-title,
     .section-footer {
-      margin-left: 24px;
+      margin: $spacer-1 $spacer-3;
     }
 
     a.section-footer {
@@ -90,12 +93,26 @@
               font-weight: $font-weight-body-regular;
             }
           }
+
+          .loading-spinner {
+            width: 24px;
+            height: 24px;
+          }
+
+          .loading-spinner,
+          .search-item-indicator {
+            margin: auto 0 auto $spacer-1;
+          }
+
+          .search-item-indicator {
+            font-style: italic;
+          }
         }
 
         /* RESULT ITEM */
         .result-item-link {
           height: 56px;
-          padding: 8px 8px 8px 32px;
+          padding: 8px 32px;
           .result-info {
             display: flex;
             flex: 1;
@@ -109,9 +126,4 @@
       }
     }
   }
-
-  .loading-spinner {
-    margin-top: 5%;
-    margin-bottom: 5%;
-  }
 }

amundsen_application/static/js/components/common/SearchBar/InlineSearchResults/tests/index.spec.tsx

@@ -309,44 +309,70 @@ describe('InlineSearchResults', () => {
     let getTotalResultsForResourceSpy;
     let mockTitle;
     let getTitleForResourceSpy;
-    beforeAll(() => {
-      const setupResult = setup();
-      props = setupResult.props;
-      wrapper = setupResult.wrapper;
-      mockResults = [
-        { href: '/test', iconClass: 'test-class', subtitle: 'subtitle', title: 'title', type: 'User' },
-        { href: '/test2', iconClass: 'test-class2', subtitle: 'subtitle2', title: 'title2', type: 'User' },
-      ]
-      getSuggestedResultsForResourceSpy = jest.spyOn(wrapper.instance(), 'getSuggestedResultsForResource').mockImplementation(() => mockResults);
-      mockTotal = 65;
-      getTotalResultsForResourceSpy = jest.spyOn(wrapper.instance(), 'getTotalResultsForResource').mockImplementation(() => mockTotal);
-      mockTitle = 'Datasets';
-      getTitleForResourceSpy = jest.spyOn(wrapper.instance(), 'getTitleForResource').mockImplementation(() => mockTitle);
-      wrapper.instance().forceUpdate();
+
+    describe('if results do not exist', () => {
+      beforeAll(() => {
+        const setupResult = setup();
+        props = setupResult.props;
+        wrapper = setupResult.wrapper;
+        mockResults = []
+        getSuggestedResultsForResourceSpy = jest.spyOn(wrapper.instance(), 'getSuggestedResultsForResource').mockImplementation(() => mockResults);
+        wrapper.instance().forceUpdate();
+      });
+
+      it('calls helper methods with given resourceType', () => {
+        getSuggestedResultsForResourceSpy.mockClear();
+        const givenResourceType = ResourceType.dashboard;
+        wrapper.instance().renderResultsByResource(givenResourceType);
+        expect(getSuggestedResultsForResourceSpy).toHaveBeenCalledWith(givenResourceType);
+      });
+
+      it('renders nothing', () => {
+        const givenResourceType = ResourceType.dashboard;
+        expect(wrapper.instance().renderResultsByResource(givenResourceType)).toBe(null);
+      });
     });
 
-    it('calls helper methods with given resourceType', () => {
-      getSuggestedResultsForResourceSpy.mockClear();
-      getTotalResultsForResourceSpy.mockClear();
-      getTitleForResourceSpy.mockClear();
-      const givenResourceType = ResourceType.dashboard;
-      wrapper.instance().renderResultsByResource(givenResourceType);
-      expect(getSuggestedResultsForResourceSpy).toHaveBeenCalledWith(givenResourceType);
-      expect(getTotalResultsForResourceSpy).toHaveBeenCalledWith(givenResourceType);
-      expect(getTitleForResourceSpy).toHaveBeenCalledWith(givenResourceType);
-    });
-
-    it('renders ResultItemList with expected props', () => {
-      const givenResourceType = ResourceType.dashboard;
-      const content = shallow(wrapper.instance().renderResultsByResource(givenResourceType));
-      const item = content.find('.inline-results-section').find(ResultItemList);
-      const itemProps = item.props();
-      expect(itemProps.onItemSelect).toEqual(props.onItemSelect);
-      expect(itemProps.resourceType).toEqual(givenResourceType);
-      expect(itemProps.suggestedResults).toEqual(mockResults);
-      expect(itemProps.totalResults).toEqual(mockTotal);
-      expect(itemProps.title).toEqual(mockTitle);
-    })
+    describe('if results exist', () => {
+      beforeAll(() => {
+        const setupResult = setup();
+        props = setupResult.props;
+        wrapper = setupResult.wrapper;
+        mockResults = [
+          { href: '/test', iconClass: 'test-class', subtitle: 'subtitle', title: 'title', type: 'User' },
+          { href: '/test2', iconClass: 'test-class2', subtitle: 'subtitle2', title: 'title2', type: 'User' },
+        ]
+        getSuggestedResultsForResourceSpy = jest.spyOn(wrapper.instance(), 'getSuggestedResultsForResource').mockImplementation(() => mockResults);
+        mockTotal = 65;
+        getTotalResultsForResourceSpy = jest.spyOn(wrapper.instance(), 'getTotalResultsForResource').mockImplementation(() => mockTotal);
+        mockTitle = 'Datasets';
+        getTitleForResourceSpy = jest.spyOn(wrapper.instance(), 'getTitleForResource').mockImplementation(() => mockTitle);
+        wrapper.instance().forceUpdate();
+      });
+
+      it('calls helper methods with given resourceType', () => {
+        getSuggestedResultsForResourceSpy.mockClear();
+        getTotalResultsForResourceSpy.mockClear();
+        getTitleForResourceSpy.mockClear();
+        const givenResourceType = ResourceType.dashboard;
+        wrapper.instance().renderResultsByResource(givenResourceType);
+        expect(getSuggestedResultsForResourceSpy).toHaveBeenCalledWith(givenResourceType);
+        expect(getTotalResultsForResourceSpy).toHaveBeenCalledWith(givenResourceType);
+        expect(getTitleForResourceSpy).toHaveBeenCalledWith(givenResourceType);
+      });
+
+      it('renders ResultItemList with expected props', () => {
+        const givenResourceType = ResourceType.dashboard;
+        const content = shallow(wrapper.instance().renderResultsByResource(givenResourceType));
+        const item = content.find('.inline-results-section').find(ResultItemList);
+        const itemProps = item.props();
+        expect(itemProps.onItemSelect).toEqual(props.onItemSelect);
+        expect(itemProps.resourceType).toEqual(givenResourceType);
+        expect(itemProps.suggestedResults).toEqual(mockResults);
+        expect(itemProps.totalResults).toEqual(mockTotal);
+        expect(itemProps.title).toEqual(mockTitle);
+      });
+    });
   });
 
   describe('renderResults', () => {
@@ -358,10 +384,9 @@ describe('InlineSearchResults', () => {
       wrapper.update();
     });
 
-    it('renders a LoadingSpinner when props.isLoading', () => {
+    it('does not render anything when props.isLoading', () => {
       const wrapper = setup({isLoading: true}).wrapper;
-      const content = shallow(wrapper.instance().renderResults());
-      expect(content.find(LoadingSpinner).exists()).toBe(true);
+      expect(wrapper.instance().renderResults()).toBe(null);
     });
 
     describe('when !props.isLoading', () => {

amundsen_application/static/js/fixtures/search/inlineResults.ts

@@ -14,6 +14,20 @@ export const isLoadingExample = {
   },
 };
 
+export const noResultsExample = {
+  isLoading: false,
+  tables: {
+    page_index: 0,
+    results: [],
+    total_results: 0,
+  },
+  users: {
+    page_index: 0,
+    results: [],
+    total_results: 0,
+  },
+};
+
 export const allResourcesExample = {
   isLoading: false,
   tables: {