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] Use unformatted number and boolean values for CSV serialization #7809

Closed
wants to merge 3 commits into from

Conversation

cherniavskii
Copy link
Member

@cherniavskii cherniavskii commented Feb 2, 2023

Extracted from #7389, necessary for #199.
Otherwise it won't be possible to properly parse these values when pasting from clipboard.

Changelog

Breaking changes

  • CSV export: number and boolean columns will export raw unformatted values instead of formatted values:

    - $11,000.05
    + 11000.05
    - yes
    + true

@cherniavskii cherniavskii added breaking change component: data grid This is the name of the generic UI component, not the React module! v6.x feature: Export labels Feb 2, 2023
@mui-bot
Copy link

mui-bot commented Feb 2, 2023

Messages
📖 Netlify deploy preview: https://deploy-preview-7809--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 614.7 1,197.5 614.7 857.18 237.054
Sort 100k rows ms 578 1,104.1 801.3 862.04 172.201
Select 100k rows ms 212.5 275.3 225.7 241.94 27.446
Deselect 100k rows ms 141.8 234.4 193.8 197.3 34.771

Generated by 🚫 dangerJS against 5df8c41

@cherniavskii cherniavskii marked this pull request as ready for review February 3, 2023 19:15
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

I'm not 100% with this change. I asked two banks in Brazil to export to CSV my account statement and both exported monetary values as "9.999,99", which is what I see in the web page. The same happened for dates, it was exported according to my locale, so "dd/mm/yyyy". Did you explore adding a valueSetter to these columns? Its purpose was exactly to convert the value entered by the user to machine format.

@cherniavskii
Copy link
Member Author

Did you explore adding a valueSetter to these columns? Its purpose was exactly to convert the value entered by the user to machine format.

I think that would work. I actually thought about adding functions that would allow transforming cell values for both import and export.

I'm not sure valueSetter is the right place for it. For example, I can add a default valueSetter for the number column type and parse the pasted value correctly, but once developers override the default valueSetter - they would lose the parsing logic.

Maybe add new functions to the column definition for import/export specifically?

@m4theushw
Copy link
Member

I'm not sure valueSetter is the right place for it. For example, I can add a default valueSetter for the number column type and parse the pasted value correctly, but once developers override the default valueSetter - they would lose the parsing logic.

They will have to override in certain occasions. For instance, I don't know if we can provide a single default valueSetter that will take care of parsing numbers/dates from their localized version back to the machine format.

Maybe add new functions to the column definition for import/export specifically?

We can do that too. AG Grid has processCellForClipboard. Adding a function exclusive for clipboard is also a way to avoid unnecessary params. The current value setter also passes a row object which is not relevant here.

@alexfauquette
Copy link
Member

@cherniavskii Since we are now in stable and it's a breaking change PR, should it be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! feature: Export v6.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants