-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Changes from 4 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,19 @@ | ||
import { GridExportCsvOptions } from '../gridExport'; | ||
|
||
/** | ||
* The csv export API interface that is available in the grid [[apiRef]]. | ||
* The CSV export API interface that is available in the grid [[apiRef]]. | ||
*/ | ||
export interface GridCsvExportApi { | ||
/** | ||
* Returns the grid data formatted as CSV. | ||
* @returns {string} The data as CSV. | ||
* Returns the grid data as a CSV string. | ||
* This method is used internally by `exportDataAsCsv`. | ||
* @param {GridExportCsvOptions} options The options to apply on the export. | ||
* @returns string | ||
*/ | ||
getDataAsCsv: () => string; | ||
getDataAsCsv: (options?: GridExportCsvOptions) => string; | ||
/** | ||
* Exports the grid data as CSV and sends it to the user. | ||
* Downloads and exports a CSV of the grid's data. | ||
* @param {GridExportCsvOptions} options The options to apply on the export. | ||
*/ | ||
exportDataAsCsv: () => void; | ||
exportDataAsCsv: (options?: GridExportCsvOptions) => void; | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,12 +1,12 @@ | ||||
/** | ||||
* Available export formats. To be extended in future. | ||||
* The options to apply on the CSV export. | ||||
*/ | ||||
export type GridExportFormat = 'csv'; | ||||
export interface GridExportCsvOptions { | ||||
fileName?: string; | ||||
utf8WithBom?: boolean; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe 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?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More context: ag-grid/ag-grid#3916 |
||||
} | ||||
|
||||
/** | ||||
* Export option interface | ||||
* Available export formats. | ||||
*/ | ||||
export interface GridExportOption { | ||||
label: React.ReactNode; | ||||
format: GridExportFormat; | ||||
} | ||||
export type GridExportFormat = '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.
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.
I assume that because as soon as we have excel, it needs:
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.
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.