Skip to content

Feat/download function #540

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

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

Feat/download function #540

wants to merge 24 commits into from

Conversation

JerryVincent
Copy link
Collaborator

Modified download feature.

Copy link
Contributor

@scheidtdav scheidtdav left a comment

Choose a reason for hiding this comment

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

Great progress so far!
I noticed a couple of small things in the code itself, please address those.

A couple more things to consider:

  1. I think all the new stuff we add should be in german and english straight away. For that reason you should add the translation function to your newly added strings
  2. May I recommend to refactor the action a little bit: Move the individual file formats into their own functions to prevent getting spaghetti code and increase reusability. E.g. one function for csv, one for json, etc. and then just call those in the action. Makes it way more readable :-)
  3. Why do I need to click the "download" button after the preparation is done? Can't this just start automatically? Would make it more user friendly in my opinion. Also can we add a note to not close the modal during the process or otherwise the download will be interrupted?
  4. If you are interested in fiddling around with performance improvements, we can have a chat about parallelizing some promises, specifically for accessing the measurements. A couple of lines including Promise.all could help speed up things for the user. Just let me know.

Copy link
Contributor

@scheidtdav scheidtdav left a comment

Choose a reason for hiding this comment

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

Still some ESLint warnings as far as I can tell.
I left two small things I found as well.

You could further increase the readability of your action by taking out the functions that generate each individual file format to a different place in the code. This way it is taken out from the reading flow.
One option would be to move it to a new file like file-exports.ts in the utils folder.

@JerryVincent JerryVincent requested a review from scheidtdav April 30, 2025 07:14
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