-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGrid] Allow to customize GridToolbarExport's CSV export #1695
[DataGrid] Allow to customize GridToolbarExport's CSV export #1695
Conversation
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
@@ -12,7 +12,7 @@ import { GridExportFormat } from '../models/gridExport'; | |||
*/ | |||
export function exportAs( | |||
blob: Blob, | |||
extension: GridExportFormat = 'csv', | |||
extension: GridExportFormatExtension = 'csv', | |||
filename: string = document.title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change the default filename to include the date.
something like export-21.05.2021-19:21.csv
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtassone This sounds off-topic to the PR, but we could create a new issue for it 👍
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
…mui#1440 This feature allows to: - set the file name - set flag utf8WithBom to generate csv file as UTF-8 with BOM It may be extended with further options in the future
d4c40b4
to
16cb13b
Compare
16cb13b
to
49cfcf0
Compare
I have pushed changes to match what the overall feedback direction seems to be
packages/grid/_modules_/grid/components/toolbar/GridToolbarExport.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/export/useGridCsvExport.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Matheus Wichman <[email protected]>
export type GridExportFormat = 'csv'; | ||
export interface GridExportCsvOptions { | ||
fileName?: string; | ||
utf8WithBom?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the withBom
suffix? I don't think it brings any value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dtassone How will a developer differentiate if he's going to include a BOM or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what they want is UTF8, the BOM part is just for the reader...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the default format? UTF8? Are developers using any other format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure actually, I thought it was ASCII and we were missing the special characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michallukowski How about we start without, only an option to customize the filename?
utf8WithBom?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main reason for this pull request was to add an option for developers to set a flag if they wanted to add a BOM for a utf-8 encoded file as mentioned in #1440. The filename setting came up by the way. What's the problem with the utf8WithBom option? If you really don't want this option, we can remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think we can default to utf8 with Bom, and either remove or reverse the prop so only users that don't want BOM for a special reason would actually set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to add a BOM by default. According to the Unicode Standard:
Use of a BOM is neither required nor recommended for UTF-8, but may be encountered in contexts where UTF-8 data is converted from other encoding forms that use a BOM or where the BOM is used as a UTF-8 signature
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More context: ag-grid/ag-grid#3916
formatOptions?: GridExportCsvOptions; | ||
} | ||
|
||
type GridExportFormatOption = GridExportFormatCsv; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need these 2 types, why not only 1?
Not sure everybody would agree but I would export the type used in the props, so users can easily type the component props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not only 1?
I assume that because as soon as we have excel, it needs:
type GridExportFormatOption = GridExportFormatCsv; | |
type GridExportFormatOption = GridExportFormatCsv | GridExportFormatExcel; |
Not sure everybody would agree but I would export the type used in the props, so users can easily type the component props.
Why would a developer ever care about them? Isn't this type completely internal to the component, with no public API implications? I mean, it seems completely dependent on how GridToolbarExport
is written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you export the button to recompose your toolbar and you allow to pass props in the button, you might want to type those props and what's inside them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you export the button to recompose your toolbar and you allow to pass props in the button, you might want to type those props and what's inside them
Then I don't understand your previous comment. The GridToolbarExportProps
type is already public for this use case.
You can use https://codesandbox.io/s/material-demo-forked-o7cee?file=/demo.tsx to provide an example if you were thinking of something else, it uses the last commit of this PR.
This feature allows to:
- set the file name
- set flag utf8WithBom to generate csv file as UTF-8 with BOM
It may be extended with further options in the future
Example how to use:
It generate file "csvWithBom.csv" with sequence of bytes (0xEF, 0xBB, 0xBF) at the beginning.
One iteration on #1440