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] Allow to set in GridExportCsvOptions delimiter #1859

Merged

Conversation

michallukowski
Copy link
Contributor

@michallukowski michallukowski commented Jun 9, 2021

Add option in GridExportCsvOptions to set delimiter character

Fix 1. of #1440

@dtassone
Copy link
Member

dtassone commented Jun 9, 2021

FYI the comma as decimal separator is according to the culture. So in the uk they use 1, 000 and in Fr, they use 1 000 I think.
So not sure we should add it as an option. Maybe we could think of an export formatter as @m4theushw mentioned somewhere (trying to recover his comment)

@michallukowski
Copy link
Contributor Author

michallukowski commented Jun 9, 2021

FYI the comma as decimal separator is according to the culture. So in the uk they use 1, 000 and in Fr, they use 1 000 I think.
So not sure we should add it as an option. Maybe we could think of an export formatter as @m4theushw mentioned somewhere (trying to recover his comment)

You're talking about the thousand separator. Using Number.prototype.toString() we don't get the thousands separator which I think we shouldn't use in CSV. For example, in many European countries, the number 3.14 in the csv file is automatically treated by Excel as a string. If the decimal separator is ',' (3,14), Excel detects as a number.
https://en.wikipedia.org/wiki/Decimal_separator

@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jun 9, 2021
@m4theushw
Copy link
Member

@michallukowski I proposed in #1814 (comment) that we apply the value formatter to the CSV export too. With it, you would be able to change how numbers are formatted when exported. This is what Damien refers to. Regarding this PR, I would avoid adding specific options to control number formatting. We could leverage Intl.NumberFormat that already provides everything. We could ask the user to pass a Intl.NumberFormat instance and use it to format values.

Using Number.prototype.toString() we don't get the thousands separator which I think we shouldn't use in CSV.

Number.prototype.toLocaleString() gives the formatted value according to the current locale.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Can we add a test case for the new delimiter param?

Using Number.prototype.toString() we don't get the thousands separator which I think we shouldn't use in CSV.

Agree, I don't see why we would want to format a number in CSV exports. It seems better to be able to rely on the CSV reader to automatically detect a number and handle it accordingly (e.g. for formatting based on the locale of the CSV reader or further math operations).

@dtassone
Copy link
Member

dtassone commented Jun 9, 2021

Yes so basically don't do anything special for numbers, we should consider using formatters on export

@michallukowski michallukowski changed the title [DataGrid] Allow to set in GridExportCsvOptions delimiter and comma as decimal separator [DataGrid] Allow to set in GridExportCsvOptions delimiter Jun 9, 2021
@DanailH
Copy link
Member

DanailH commented Jun 11, 2021

Can we add a test case for the new delimiter param?

Using Number.prototype.toString() we don't get the thousands separator which I think we shouldn't use in CSV.

Agree, I don't see why we would want to format a number in CSV exports. It seems better to be able to rely on the CSV reader to automatically detect a number and handle it accordingly (e.g. for formatting based on the locale of the CSV reader or further math operations).

This reminds me that I don't think that the CSV export currently uses the valueFormatter although it does use valueGetter.

@oliviertassinari oliviertassinari requested a review from a team June 13, 2021 17:23
@dtassone dtassone merged commit d392425 into mui:master Jun 16, 2021
@michallukowski michallukowski deleted the my-grid-toolbar-export-csv-separator branch June 16, 2021 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants