Repository: gregnb/mui-datatables
Pull request #1073 (2019-11-21 22:49:12)

Deprecation warnings
Currently there are some deprecation warnings that show up in production code, not sure what your opinion is @gabrielliwerant, but I think its enough to know about the deprecations when developing locally, no need for error reporters to see it I think.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/27159264/badge)](https://coveralls.io/builds/27159264)

Coverage decreased (-0.4%) to 76.938% when pulling **8ccd8a55538df0bc95cdbe1c3624304f857ffe2d on jdetle:deprecation-warnings** into **2d37c383beeebb039fae2b6e0b531ddb7959e2ce on gregnb:master**.

gabrielliwerant: Hey @jdetle, I think that hiding warnings in production environments makes sense. However, I think it might be nice to have a higher level abstraction around this so that's easier to use in future cases, and we're less likely to forget to add it. We can also use that to tweak the logic more easily in the future if we need to as well.

One idea would be to send all warnings to a warning function before logging them, but I'm open to other suggestions. What do you think?
jdetle: My favorite experiences have been when the deprecation error is a warning that will occur once per session, multiple uses of the table in one session shouldn't show the warning again IMO. We could have 'warn' utility function somewhere that accepts an id for a deprecation warning, sends the warning, and records that in a global object somewhere so that future calls to the function dont do anything if that id has already been recorded. What do you think?
gabrielliwerant: I don't think we should only show the warnings once per session. They will be too easy to miss for developers that don't know to look for them or to expect once-only behavior. I think hiding in production is good, but the warnings are important, and I do want developers to be bothered by them so that they will fix them rather than ignoring them.
jdetle: Okay then in that case just special log function imported from a util file somewhere that hides the message if its in prod?
gabrielliwerant: Yes @jdetle, that sounds good to me!
jdetle: Okay, made an attempt, let me know if you want to change the naming
jdetle: @gabrielliwerant, I made the changes, hopefully they meet your expectations :D 
gabrielliwerant: @jdetle Thanks for making those changes! One more thing. I didn't notice this last time, but since I'm able to test it now, I realized that you changed the errors to warnings. I think they should stay errors. My main priority here is to ensure that devs are adequately notified about breaking changes and I think warnings are much easier to ignore. Additionally, material-ui uses the convention of errors so I think it would be good to stick with how they do it since this library is built to work with it.
jdetle: Done.
jdetle: Not used to this workflow, whats in the way of this getting merged now?
gabrielliwerant: Nothing, I just have to create the release and document everything. Sometimes I like to put in a certain number of changes at once so I don't have the overhead of creating lots of releases, it depends on numerous factors.

Patch:
diff --git a/src/MUIDataTable.js b/src/MUIDataTable.js
index 60af68f..ad9fd3d 100644
--- a/src/MUIDataTable.js
+++ b/src/MUIDataTable.js
@@ -17,7 +17,7 @@ import TableResize from './components/TableResize';
 import TableToolbar from './components/TableToolbar';
 import TableToolbarSelect from './components/TableToolbarSelect';
 import getTextLabels from './textLabels';
-import { buildMap, getCollatorComparator, sortCompare, getPageValue } from './utils';
+import { buildMap, getCollatorComparator, sortCompare, getPageValue, warnDeprecated } from './utils';
 
 const defaultTableStyles = theme => ({
   root: {},
@@ -332,28 +332,28 @@ class MUIDataTable extends React.Component {
 
   handleOptionDeprecation = () => {
     if (typeof this.options.selectableRows === 'boolean') {
-      console.error(
+      warnDeprecated(
         'Using a boolean for selectableRows has been deprecated. Please use string option: multiple | single | none',
       );
       this.options.selectableRows = this.options.selectableRows ? 'multiple' : 'none';
     }
     if (['scrollMaxHeight', 'scrollFullHeight', 'stacked'].indexOf(this.options.responsive) === -1) {
-      console.error(
+      warnDeprecated(
         'Invalid option value for responsive. Please use string option: scrollMaxHeight | scrollFullHeight | stacked',
       );
     }
     if (this.options.responsive === 'scroll') {
-      console.error('The "scroll" responsive option has been deprecated. It is being replaced by "scrollMaxHeight"');
+      warnDeprecated('This option has been replaced by scrollMaxHeight');
     }
     if (this.options.fixedHeader === false || this.options.fixedHeader) {
-      console.error(
+      warnDeprecated(
         'fixedHeader has been deprecated in favor of fixedHeaderOptions: { xAxis: boolean, yAxis: boolean }. Once removed, the new options will be set by default to render like the old fixedHeader. However, if you are setting the fixedHeader value manually, it will no longer work in the next major version.',
       );
     }
 
     this.props.columns.map(c => {
       if (c.options && c.options.customFilterListRender) {
-        console.error(
+        warnDeprecated(
           'The customFilterListRender option has been deprecated. It is being replaced by customFilterListOptions.render (Specify customFilterListOptions: { render: Function } in column options.)',
         );
       }
@@ -377,8 +377,8 @@ class MUIDataTable extends React.Component {
       throw Error('renderExpandableRow must be provided when using expandableRows option');
     }
     if (this.props.options.filterList) {
-      console.error(
-        'Deprecated: filterList must now be provided under each column option. see https://github.com/gregnb/mui-datatables/tree/master/examples/column-filters example',
+      warnDeprecated(
+        'filterList must now be provided under each column option. see https://github.com/gregnb/mui-datatables/tree/master/examples/column-filters example',
       );
     }
   }
@@ -447,7 +447,7 @@ class MUIDataTable extends React.Component {
           }
 
           if (options.sortDirection === null) {
-            console.error(
+            warnDeprecated(
               'The "null" option for sortDirection is deprecated. sortDirection is an enum, use "asc" | "desc" | "none"',
             );
             options.sortDirection = 'none';
@@ -502,8 +502,8 @@ class MUIDataTable extends React.Component {
         data => data.filter(d => typeof d === 'object' && d !== null && !Array.isArray(d)).length > 0,
       ).length > 0;
     if (hasInvalidData)
-      console.error(
-        'Deprecated: Passing objects in as data is not supported, and will be prevented in a future release. Consider using ids in your data and linking it to external objects if you want to access object data from custom render functions.',
+      warnDeprecated(
+        'Passing objects in as data is not supported, and will be prevented in a future release. Consider using ids in your data and linking it to external objects if you want to access object data from custom render functions.',
       );
 
     return transformedData;
@@ -556,8 +556,8 @@ class MUIDataTable extends React.Component {
       if (column.filterOptions) {
         if (Array.isArray(column.filterOptions)) {
           filterData[colIndex] = cloneDeep(column.filterOptions);
-          console.error(
-            'Deprecated: filterOptions must now be an object. see https://github.com/gregnb/mui-datatables/tree/master/examples/customize-filter example',
+          warnDeprecated(
+            'filterOptions must now be an object. see https://github.com/gregnb/mui-datatables/tree/master/examples/customize-filter example',
           );
         } else if (Array.isArray(column.filterOptions.names)) {
           filterData[colIndex] = cloneDeep(column.filterOptions.names);
@@ -1336,7 +1336,7 @@ class MUIDataTable extends React.Component {
     let responsiveClass;
 
     switch (this.options.responsive) {
-      // DEPRECATED: This options is beign transitioned to `responsiveScrollMaxHeight`
+      // DEPRECATED: This options is being transitioned to `responsiveScrollMaxHeight`
       case 'scroll':
         responsiveClass = classes.responsiveScroll;
         break;
diff --git a/src/components/TableBody.js b/src/components/TableBody.js
index bc618ab..e7aa9f6 100644
--- a/src/components/TableBody.js
+++ b/src/components/TableBody.js
@@ -7,7 +7,7 @@ import TableBodyRow from './TableBodyRow';
 import TableSelectCell from './TableSelectCell';
 import { withStyles } from '@material-ui/core/styles';
 import cloneDeep from 'lodash.clonedeep';
-import { getPageValue } from '../utils';
+import { getPageValue, warnDeprecated } from '../utils';
 
 const defaultBodyStyles = {
   root: {},
@@ -167,8 +167,8 @@ class TableBody extends React.Component {
       // In a future release, onRowClick will no longer be called here (for consistency).
       // For now, issue a deprecated warning.
       if (this.props.options.onRowClick) {
-        console.warn(
-          'Deprecated: Clicks on expandable button will not trigger onRowClick in an upcoming release, see: https://github.com/gregnb/mui-datatables/issues/516.',
+        warnDeprecated(
+          'Clicks on expandable button will not trigger onRowClick in an upcoming release, see: https://github.com/gregnb/mui-datatables/issues/516.',
         );
         this.props.options.onRowClick(row, data, event);
       }
diff --git a/src/components/TableToolbar.js b/src/components/TableToolbar.js
index c26be05..99ccbfc 100644
--- a/src/components/TableToolbar.js
+++ b/src/components/TableToolbar.js
@@ -178,7 +178,12 @@ class TableToolbar extends React.Component {
   };
 
   getActiveIcon = (styles, iconName) => {
-    return this.state.iconActive !== iconName ? styles.icon : styles.iconActive;
+    let isActive = this.state.iconActive === iconName;
+    if (iconName === 'search') {
+      const { showSearch, searchText } = this.state;
+      isActive = isActive || showSearch || searchText;
+    }
+    return isActive ? styles.iconActive : styles.icon;
   };
 
   showSearch = () => {
@@ -208,6 +213,15 @@ class TableToolbar extends React.Component {
     this.props.searchTextUpdate(value);
   };
 
+  handleSearchIconClick = () => {
+    const { showSearch, searchText } = this.state;
+    if (showSearch && !searchText) {
+      this.hideSearch();
+    } else {
+      this.setActiveIcon('search');
+    }
+  }
+
   render() {
     const {
       data,
@@ -258,7 +272,7 @@ class TableToolbar extends React.Component {
                 data-testid={search + '-iconButton'}
                 buttonRef={el => (this.searchButton = el)}
                 classes={{ root: this.getActiveIcon(classes, 'search') }}
-                onClick={this.setActiveIcon.bind(null, 'search')}>
+                onClick={this.handleSearchIconClick}>
                 <SearchIcon />
               </IconButton>
             </Tooltip>
diff --git a/src/utils.js b/src/utils.js
index 3eb2c46..2b3f26f 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -5,6 +5,12 @@ function buildMap(rows) {
   }, {});
 }
 
+function warnDeprecated(warning) {
+  if (process.env.NODE_ENV === 'development') {
+    console.error(`Deprecation Notice:  ${warning}`);
+  }
+}
+
 function getPageValue(count, rowsPerPage, page) {
   const totalPages = count <= rowsPerPage ? 1 : Math.ceil(count / rowsPerPage);
 
@@ -43,7 +49,7 @@ function buildCSV(columns, data, options) {
         .reduce(
           (soFar, column) =>
             column.download
-              ? soFar + '"' + replaceDoubleQuoteInString(column.name) + '"' + options.downloadOptions.separator
+              ? soFar + '"' + replaceDoubleQuoteInString(column.label || column.name) + '"' + options.downloadOptions.separator
               : soFar,
           '',
         )
@@ -108,4 +114,13 @@ function createCSVDownload(columns, data, options, downloadCSV) {
   downloadCSV(csv, options.downloadOptions.filename);
 }
 
-export { buildMap, getPageValue, getCollatorComparator, sortCompare, createCSVDownload, buildCSV, downloadCSV };
+export {
+  buildMap,
+  getPageValue,
+  getCollatorComparator,
+  sortCompare,
+  createCSVDownload,
+  buildCSV,
+  downloadCSV,
+  warnDeprecated,
+};


Test patch:
diff --git a/test/MUIDataTableToolbar.test.js b/test/MUIDataTableToolbar.test.js
index ec4ffb6..9cdaf62 100644
--- a/test/MUIDataTableToolbar.test.js
+++ b/test/MUIDataTableToolbar.test.js
@@ -193,6 +193,75 @@ describe('<TableToolbar />', function() {
     assert.strictEqual(actualResult.length, 0);
   });
 
+  it('should hide search when search icon is clicked while search is open without content', () => {
+    const searchTextUpdate = () => {};
+    const shallowWrapper = shallow(
+      <TableToolbar
+        searchClose={() => {}}
+        searchTextUpdate={searchTextUpdate}
+        columns={columns}
+        data={data}
+        options={options}
+        setTableAction={setTableAction}
+      />,
+    ).dive();
+    const instance = shallowWrapper.instance();
+    instance.searchButton = {
+      focus: () => {},
+    };
+
+    // click search button to display search
+    shallowWrapper.find('[data-testid="Search-iconButton"]').simulate('click');
+    shallowWrapper.update();
+
+    assert.strictEqual(shallowWrapper.state('iconActive'), 'search');
+    let actualResult = shallowWrapper.find(TableSearch);
+    assert.strictEqual(actualResult.length, 1);
+
+    // now click search button again and test
+    shallowWrapper.find('[data-testid="Search-iconButton"]').simulate('click');
+    shallowWrapper.update();
+
+    assert.strictEqual(shallowWrapper.state('iconActive'), null);
+    actualResult = shallowWrapper.find(TableSearch);
+    assert.strictEqual(actualResult.length, 0);
+  });
+
+  it('should not hide search when search icon is clicked while search is open with content', () => {
+    const searchTextUpdate = () => {};
+    const shallowWrapper = shallow(
+      <TableToolbar
+        searchClose={() => {}}
+        searchTextUpdate={searchTextUpdate}
+        columns={columns}
+        data={data}
+        options={options}
+        setTableAction={setTableAction}
+      />,
+    ).dive();
+    const instance = shallowWrapper.instance();
+    instance.searchButton = {
+      focus: () => {},
+    };
+
+    // click search button to display search
+    shallowWrapper.find('[data-testid="Search-iconButton"]').simulate('click');
+    shallowWrapper.update();
+
+    assert.strictEqual(shallowWrapper.state('iconActive'), 'search');
+    let actualResult = shallowWrapper.find(TableSearch);
+    assert.strictEqual(actualResult.length, 1);
+
+    // now set searchText and click search button again and test
+    shallowWrapper.setState({ searchText: 'fakeSearchText'});
+    shallowWrapper.find('[data-testid="Search-iconButton"]').simulate('click');
+    shallowWrapper.update();
+
+    assert.strictEqual(shallowWrapper.state('iconActive'), 'search');
+    actualResult = shallowWrapper.find(TableSearch);
+    assert.strictEqual(actualResult.length, 1);
+  });
+
   it('should call onFilterDialogOpen when opening filters via toolbar', () => {
     const onFilterDialogOpen = spy();
     const newOptions = { ...options, onFilterDialogOpen };
@@ -236,6 +305,26 @@ describe('<TableToolbar />', function() {
     assert.strictEqual(state.iconActive, 'filter');
   });
 
+  it('should render search icon as active if option.searchOpen = true', () => {
+    const newOptions = { ...options, search: true, searchOpen: true };
+    const shallowWrapper = shallow(
+      <TableToolbar columns={columns} data={data} options={newOptions} setTableAction={setTableAction} />,
+    ).dive();
+    const actualResult = shallowWrapper.find('[data-testid="Search-iconButton"]');
+    assert.strictEqual(actualResult.prop('classes').root.indexOf('MUIDataTableToolbar-iconActive-'), 0);
+
+  });
+
+  it('should render search icon as active if option.searchText = some_text', () => {
+    const newOptions = { ...options, search: true, searchText: 'searchText' };
+    const shallowWrapper = shallow(
+      <TableToolbar columns={columns} data={data} options={newOptions} setTableAction={setTableAction} />,
+    ).dive();
+    const actualResult = shallowWrapper.find('[data-testid="Search-iconButton"]');
+    assert.strictEqual(actualResult.prop('classes').root.indexOf('MUIDataTableToolbar-iconActive-'), 0);
+
+  });
+
   it('should download CSV when calling method handleCSVDownload', () => {
     const shallowWrapper = shallow(
       <TableToolbar