-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add task configurations download as csv #4491
Add task configurations download as csv #4491
Conversation
@normanrz I'll unassign you as @philippotto is available |
@philippotto Could you please review this PR? I just added the changelog entry and the code should be cleaned up 🧹 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff! However, see my few suggestions. Most notably, the format of the CSV should match the existing format which is currently shown in the modal.
Also, I think that constructing a CSV does not make much sense if it contains only one line. To solve this, I'd simply show a modal (as before) with the CSV content (only if tasks.length < 50 or so) and an additional "download as csv" button. If there are more than 50 tasks, the modal should still open and it would say "too many tasks to display in the browser. please download as csv" or something like that.
This has the additional benefit, that the user is in control over the download (i.e., they have to click a download button) and not surprised by it (or even worse he might even miss that something was downloaded).
Sorry for the change of plans, but I hope that it's not too much trouble :)
…-task-settings-as-csv-download
@philippotto Thanks for your feedback 👍
No problem, this does not bother me. My main goal is to have clean code and the best UX. I missed that some browsers are configured to automatically download files without any pop-up. In my case firefox always opens a dialog and asks whether to download a file. That's why I did not see this as a problem :) |
@philippotto The newest version should include all of your feedback. Could you please have a look at the changes again 😄? |
* remove individual download in task list view
This file might be useful for testing the bulk creation. It should contain some valid tasks and some broken ones 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! This will be a pretty nice update for the admins :)
I left some minor remarks, and I also have some suggestions to make the UI a bit nicer:
in summary:
- bold labels
- max-height: 300px and overflow: auto
- right-aligned download buttons
- spacer
Also, the UI for the "too many tasks" case looks a bit unpolished:
I suggest to change it like this (note that I centered the download button, since it's the main content of that section):
handleFormSubmit = (isRandom: boolean, event: ?SyntheticInputEvent<*>) => { | ||
handleFormSubmitWithSubsequentCall = ( | ||
isRandom: boolean, | ||
subsequentMethod: QueryObject => Promise<void>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subsequentMethod
is quite an unusual name, in my opinion. I'd go with onFinish
, onFinishCallback
or onFinishFunction
instead. Also, the method name can remain at handleFormSubmit
, since the method name doesn't need to explain its parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you noticed correctly, I had problems with finding a good naming here. 😕
The reason why I renamed the handleFormSubmit
method is, that this name was already used in line 142.
But in my opinion, the naming should be fine now in the newest changes. 👍
Thanks a lot for your very detailed feedback and also for the very good suggestions on how to improving the UI 😄 The look of the current version: @philippotto Could you please review the newest changes / version of this PR? (I hope I did not miss something 🙈 ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome :) 🕺
This PR adds the download one or multiple task configurations as csv to wk. The download is available
URL of deployed dev instance (used for testing):
Steps to test:
Issues:
[ ] Updated migration guide if applicable[ ] Updated documentation if applicable[ ] Adapted wk-connect if datastore API changes[ ] Needs datastore update after deployment