Repository: gregnb/mui-datatables
Pull request #1059 (2019-11-11 13:16:41)

Fix(#444): downloaded csv should show column "label", not "name"
See #444 

Here's an example to look at as well, with the latest `@material-ui/core`, `mui-datatables`, and `react` packages: https://codesandbox.io/s/muidatatables-custom-toolbar-dzhnx

After downloading the csv file, notice how column labels are all lower-case, as they are in the `'name'` properties of the column options, instead of the `'label'` property:

![image](https://user-images.githubusercontent.com/3885503/68589866-bfa32600-0495-11ea-9f89-639810fba515.png)

Near the bottom of the `buildColumns` method of the `MUIDataTable` class, you can see that the `label` column option will always populated, with its default being the column `name` (if that column's options is an object) or just that column string:

![image](https://user-images.githubusercontent.com/3885503/68590026-33453300-0496-11ea-8eb5-8cdc5740dd81.png)

Beamanator: I don't believe this warrants an extra setting in a column's `downloadOptions`, but I'd be happy to think about that case if y'all prefer.
coveralls: 
[![Coverage Status](https://coveralls.io/builds/26909108/badge)](https://coveralls.io/builds/26909108)

Coverage remained the same at 77.308% when pulling **28650d451623ff91f159fe104401f4b1dc723c85 on Beamanator:master** into **2d37c383beeebb039fae2b6e0b531ddb7959e2ce on gregnb:master**.

Beamanator: @gabrielliwerant I was thinking the following referenced line prevented `column.label` (in [`buildHead`](https://github.com/gregnb/mui-datatables/blob/12b2d2baadd2fe4815ea04fe9f9240a1687876d7/src/utils.js#L40)) from ever *not existing*, since it copies `column.name` into `label` if `column.label` doesn't exist (and it seems like this is where the main column data gets built and used in the rest of the library):

https://github.com/gregnb/mui-datatables/blob/a7db00268e4498fcee93f87b0ac8c1d95a5f4a9f/src/MUIDataTable.js#L468

However, maybe I'm missing another use case where `column.label` can be `undefined`. Either way, It's an easy fix to just use `column.name` if `column.label` doesn't exist - as you mentioned, and as seen in the latest commit. Hope this is what you were thinking!
gabrielliwerant: @Beamanator You are right that the table sets `label` as `name` when none exists and you are using the table normally. However, when `buildCSV` is used/tested in isolation, it breaks without the default being set. I think it's good practice to keep as many pieces as possible working in isolation so that they could be used on their own if necessary, and not be dependent on side effects which are difficult to test and maintain.

To that end, your new commit is perfect, thank you!
thclark: Thanks to you both, @Beamanator and @gabrielliwerant for your work here :)

I'm conscious that I'm kool-aid-guy-ing in here, and am sorry for that. But, I think it would be very helpful to have this as an option (I'd suggest it's introduced as default off, then at the next major version release changed to default on), because:
 - This is a breaking change for anyone who uses those downloaded sheets in any kind of automation or audit process
- This breaks the exported columns wherever a `label` contains `options.downloadOptions.separator`
- This potentially breaks data integrity/auditability, since there's no export of the actual data field names

Does it seem like a major change to make it an option?
gabrielliwerant: Hey @thclark. Kool-aid guy references make everything ok. :)

What is the feature to which you are referring? The issue here is about label vs name showing in the exported csv, which is a bug and not a breaking change. Is there something I'm missing?
thclark: Hi @gabrielliwerant

So, the first aspect was really asking the question "what do the end users do with the CSV files once they've downloaded them". If they have some kind of workflow based on those exports (like saving to a dropbox folder zaps the contents to a google sheet, for/ex), the workflow gets broken.

The second aspect was let's say we have columns:
```
[{
name: "wood_petrified",
label: "Wood, petrified"
},
{
name: "wood_fossilised",
label: "Wood, fossilised", 
}]
```
The current implementation will output the CSV:
```
Wood, petrified, Wood, fossilised
value1a, value1b
value2b, value2b
```

Of those two scenarios, the second is more serious since it's actually a broken export (as opposed to the first not being a breaking change here, but potentially meaning downstream workflows get broken).

Entirely your call but thought it worth mentioning these edge cases :)

gabrielliwerant: @thclark I appreciate you keeping a lookout, but I don't think anything gets broken here. All the fields of data in the export are quoted, so there is not confusion between the separator and actual values. For example, using commas in the label as you show, results in:
```
"Wood, petrified", "Wood, fossilised"
"value1a", "value1b "
...
```

Patch:
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..1f28beb 100644
--- a/src/utils.js
+++ b/src/utils.js
@@ -43,7 +43,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,
           '',
         )


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