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

Extended downloadCSV helper with type paramater #5499

Merged
merged 4 commits into from
Nov 13, 2020

Conversation

ValentinnDimitroff
Copy link
Contributor

Recently I needed to change the output encoding (text/csv;charset=utf-8) of the csv file. I had to reimplement it at my side because currently there is no way to plug in at the type sport and customize the output.

That's why I am proposing this extended parameter with default value if nothing is passed in.

Recently I needed to change the output encoding (text/csv;charset=utf-8) of the csv file. I had to reimplement it at my side because currently there is no way to  plug in at the type sport and customize the output. 

That's why I am proposing this extended parameter with default value if nothing is passed in.
@fzaninotto
Copy link
Member

Shouldn't text/csv;charset=utf-8 be the default mime type? After all, the output always uses UTF-8, right?

@ValentinnDimitroff
Copy link
Contributor Author

Actually I also think it is better to be the default mime type. In my case it appears it was not the default behaviour because I had unreadble cyrilic symbols in the exported csv file. After the change it all started working correctly.

I just wanted to keep everything as it was but if you don't mind I can replace the parameter's default value and update the PR?

@fzaninotto
Copy link
Member

yes, please!

@ValentinnDimitroff ValentinnDimitroff changed the base branch from master to next November 11, 2020 23:27
@fzaninotto
Copy link
Member

I didn't make myself clear enough: I think we don't need to make the type customizable is we modify it to always be utf-8. So you could just make it a bug fix instead of an enhancement.

@ValentinnDimitroff ValentinnDimitroff changed the base branch from next to master November 12, 2020 20:48
@ValentinnDimitroff
Copy link
Contributor Author

Yeah, sounds as the most sensible solution. Didn't catch your point the first time - I rebased to master now as it is just a bug fix.

@djhi djhi added this to the 3.10.1 milestone Nov 13, 2020
@djhi djhi merged commit 1c85078 into marmelab:master Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants