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

fix: escape cells of CSV download #196

Closed
wants to merge 0 commits into from
Closed

Conversation

jase88
Copy link
Contributor

@jase88 jase88 commented Apr 24, 2024

  • escaping every CSV cell (untouched if not necessary)
  • ensuring proper file name of CSV to download
  • made sure texts are localized
  • add unit test framework jest with Angulars experimental builder @angular-devkit/build-angular:jest
    • alternatively we could also
      • a) use jest-preset-angular (which would add another package and some more configuration files)
      • b) use vitest via @analogjs/vite-plugin-angular (which would add a layer of analogjs for testing)
    • happy to discuss this

fixes #69

@jase88
Copy link
Contributor Author

jase88 commented Apr 30, 2024

@tjorbo ready for review or happy to discuss.
If necessary, I can also outsource things to separate PRs.

Bumping rxjs for example is also done here: #200

@tjorbo tjorbo self-requested a review April 30, 2024 11:18
@hassmal hassmal self-requested a review May 6, 2024 13:18
Copy link
Member

@tjorbo tjorbo left a comment

Choose a reason for hiding this comment

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

Hi @jase88,
Thank you for your PR.

I have reviewed your changes and agree with most of them.

Most of my requested changes are related to translations. Nice idea to translate the csv file as well. But is it possible to move the translation properties so we don't have to add a csvExport property?

Also, I found a serious bug while testing your changes. #203 It is probably not a bug in your code, but if you could do a rebase, it would be clearer which changes are yours. We want to prevent merge commits by rebasing in general. So I will wait with rebasing dependabot's PRs until your PR is done. (To avoid conflicts in package.json)

Thanks again for your contribution.

src/app/poll/csv-utils.spec.ts Outdated Show resolved Hide resolved
docker-replace-parameters.sh Outdated Show resolved Hide resolved
src/app/poll/poll-form-helper.service.ts Outdated Show resolved Hide resolved
src/locales/de-DE-du.json Outdated Show resolved Hide resolved
src/app/poll/poll-form-helper.service.ts Outdated Show resolved Hide resolved
src/locales/de-DE-du.json Outdated Show resolved Hide resolved
src/locales/de-DE-sie.json Outdated Show resolved Hide resolved
src/locales/en-EN.json Outdated Show resolved Hide resolved
src/locales/platt.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@tjorbo tjorbo added the bug Something isn't working label May 6, 2024
src/locales/platt.json Outdated Show resolved Hide resolved
src/locales/platt.json Outdated Show resolved Hide resolved
src/locales/platt.json Outdated Show resolved Hide resolved
@tjorbo
Copy link
Member

tjorbo commented May 8, 2024

@jase88 I couldn't rebase the branch onto main because of conflicts. I fixed them by rebasing jase88:main onto terminfinder-frontend:main and rebuilding the package-lock. You can force push this branch to your repo or resolve the conflicts yourself.

Thank you for your patience 👍

@jase88
Copy link
Contributor Author

jase88 commented May 15, 2024

@tjorbo
Also thank you for your patience and helping out with the mixed branch commits.

force pushing to my fork automatically closed this PR, wasn't aware of that.

So I pushed the branch you mentioned to my fork and created a new PR: #209

Hope thats fine.

@tjorbo
Copy link
Member

tjorbo commented May 15, 2024

@jase88 I didn't know that either. Just merged #209. Thanks for the first contribution to this project. ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Commas in names break the CSV export
3 participants