Skip to content
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] Make the CSV export respect the valueFormatter #1922

Merged

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jun 17, 2021

Fix #1814

@DanailH DanailH changed the title Make the CSV export respect zvalueFormatter` [DataGrid] Make the CSV export respect the valueFormatter Jun 17, 2021
@DanailH DanailH self-assigned this Jun 17, 2021
@DanailH DanailH added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels Jun 17, 2021
delimiterCharacter: GridExportCsvDelimiter;
}

export function buildCSV(options: BuildCSVOptions): string {
const { columns, rows, selectedRows, getCellValue, delimiterCharacter } = options;
const { columns, rows, selectedRows, getCellParams, delimiterCharacter } = options;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make it an opt out option?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applying the same logic then we should also make delimiterCharacter optional. Although you need either 'getCellParams' or getCellValue here to get the actual cell value because we only have access to the cell ID and using the apiRef in here is not ideal as that file is with helper/utility functions that are grid agnostic.
IMO we can keep it for now as it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's ok, I meant using the value formatter, there might be cases where one would prefer the raw value, maybe it should be column based as well 🤔
What do you think? @mui-org/x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first step was to apply the same valueFormatter to the CSV export, that might be sufficient for most of the use cases. If someone requests for a per-column formatting, we can add an exclusive value formatter for the export: #1814 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's what we said originally. I'm just being extra cautious here about numbers, ie 1,000, should be exported as "1,000", would that be interpreted as numbers by excel? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested and it seems ok. Let's not do the option for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we had a PR about that: #1656 (comment) If the OS is correctly configured, we just need to use the same formatting in the CSV export.

@DanailH DanailH requested a review from dtassone June 18, 2021 08:06
delimiterCharacter: GridExportCsvDelimiter;
}

export function buildCSV(options: BuildCSVOptions): string {
const { columns, rows, selectedRows, getCellValue, delimiterCharacter } = options;
const { columns, rows, selectedRows, getCellParams, delimiterCharacter } = options;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first step was to apply the same valueFormatter to the CSV export, that might be sufficient for most of the use cases. If someone requests for a per-column formatting, we can add an exclusive value formatter for the export: #1814 (comment)

@DanailH DanailH merged commit 9904b5e into mui:master Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] valueGetter does not apply to CSV export
3 participants