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

Insert BOM to encode CSV correctly #2742

Merged
merged 1 commit into from
Oct 15, 2019

Conversation

Gaetanbrl
Copy link
Contributor

@Gaetanbrl Gaetanbrl commented Oct 7, 2019

This PR specify the encoding to display specials characters (é,à,ù,...) for user(s) export only.

Link to #2737 .

@fvanderbiest
Copy link
Member

BOMs are a nightmare ...
https://fr.wikipedia.org/wiki/Indicateur_d%27ordre_des_octets#UTF-8

What if the user manually selects UTF-8 when importing the file in LibreOffice?
I guess it fixes the issue.

@Gaetanbrl
Copy link
Contributor Author

What if the user manually selects UTF-8 when importing the file in LibreOffice?

Not easy to do for each user with Excel, that's suppose to use LibreOffice only.

@fvanderbiest
Copy link
Member

fvanderbiest commented Oct 8, 2019

Excel

Are there people still using it ?
Just kidding ;-)

OK, I'd like to collect other people's opinion here before deciding to merge it or not.
@landryb @pmauduit @RemiDesgrange @cmangeat Do you think it's a good idea to create invalid UTF-8 documents to please Excel and ease user experience ?

@fvanderbiest
Copy link
Member

@Gaetanbrl
Copy link
Contributor Author

Gaetanbrl commented Oct 8, 2019

BOMs are a nightmare ...

You're right, it's maybe not the best way...

@RemiDesgrange
Copy link
Contributor

I'm noy using windows, but normally, if you send the right headers (Content-Type: text/csv; charset=utf-8 it should work ? I never set the BOM when downloading files, and Excel user were not disturbed 🤔 There should have a better way

@@ -96,7 +96,7 @@ class UsersController {
default:
mimetype = `text/${fileType}`
}
const blob = new Blob([result.data], { type: mimetype })
const blob = new Blob(['\ufeff', result.data], { type: mimetype })
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe

const blob = new Blob(['\ufeff', result.data], { type: `mimetype; charset=utf-8` })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but text/csv; charset=utf-8 don't work.

Copy link
Contributor

@RemiDesgrange RemiDesgrange left a comment

Choose a reason for hiding this comment

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

I don't think using BOM is the right solution IMO

@fvanderbiest
Copy link
Member

If we all agree it's not a good idea, let's close this one.

@landryb
Copy link
Member

landryb commented Oct 10, 2019

I will always agree on

BOMs are a nightmare ...

and actually would expand it to

encodings are a nightmare

@fvanderbiest fvanderbiest reopened this Oct 15, 2019
@fvanderbiest
Copy link
Member

Directly from the trenches... I can confirm that it's a nightmare for Excel users to open the file.
So I'm +1 to add the BOM, I finally made my mind.

@fvanderbiest fvanderbiest merged commit a352710 into georchestra:master Oct 15, 2019
@Gaetanbrl Gaetanbrl deleted the issue-2737 branch October 16, 2019 09:37
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.

None yet

4 participants