-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
[#48274] Work packages export modal with settings & Job status modal #16364
[#48274] Work packages export modal with settings & Job status modal #16364
Conversation
Step 1: Turbo- and primerize the modal https://community.openproject.org/work_packages/48274
frontend/src/stimulus/controllers/dynamic/work-packages/export/form.controller.ts
Fixed
Show fixed
Hide fixed
...nd/src/app/shared/components/op-context-menu/handlers/op-settings-dropdown-menu.directive.ts
Fixed
Show fixed
Hide fixed
...nd/src/app/shared/components/op-context-menu/handlers/op-settings-dropdown-menu.directive.ts
Fixed
Show fixed
Hide fixed
...nd/src/app/shared/components/op-context-menu/handlers/op-settings-dropdown-menu.directive.ts
Fixed
Show fixed
Hide fixed
resolves #16364 (comment)
…ove it into the form.
Thanks a lot for the very good feedback and detailed report, @HDinger! I addresses & fixed nearly all of them, except the very first.
A background job status only has one message while processing, and that is already used by „Work packages are being exported“.
I keep trying to reproduce the column error you had, because that looks suspicious and should® not happen at all. Thanks again! 💙 Update: Found the bug. Column selection shows all available columns, including custom fields that are not valid in a project scoped query. I will filter them out. |
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.
I pulled again shortly before writing this, @as-op . The latest commits seem to have fixed the issue about the d&d on the column selection. As such, I only have comments now and the PR can be merged in my opinion.
I still think though, that additional work would help improving the export modal as when using it, I found a couple of UI/UX issues (in my opinion):
- The text below the "Add column text" states that I cannot add long text fields in table view. But when switching to Report, that limitation is no longer mentioned. But I still cannot add the long text columns there but need to scroll down (which is easy to overlook) and then add the long text fields as a separate element. A bit of explanatory text, also as to that I cannot have the long text fields mixed with the other columns is missing.
- The subtext below the "Add columns" makes it harder to understand (for me) that the next element in which the columns can be sorted are affected by it as well and that the two belong together. Would it be possible to have a section heading? Ideally we would have a selection that also allows to sort the elements but I understand that this is out of scope.
- The ng-select of the "Add columns" are oftentimes cut off and require the user to scroll to see all options. (e.g. CSV format)
- The options to export "Table", "Report" and "Gantt chart" are the same for both the "work packages" as well as for the "Gantt charts" module. That is confusing to me as I would have expected to only have the "Gantt chart" option when exporting in "Gantt" (or as it is the only option then, no option at all). And when exporting in "Work packages", I would have expected to only have "Table" and "Report". From my personal perspective, I would much rather we readd the option to activate the Gantt chart to the work package list as well but the current decision is towards the other direction.
My review focuses on the backend as I understand that @HDinger already covered the frontend.
app/components/work_packages/exports/pdf/gantt/export_settings_component.rb
Show resolved
Hide resolved
frontend/src/stimulus/controllers/dynamic/job-status-polling.controller.ts
Outdated
Show resolved
Hide resolved
modules/job_status/app/components/job_status/dialog/dialog_component.html.erb
Outdated
Show resolved
Hide resolved
Hi @ulferts, I can agree with the UX remarks. I will copy the points to the work package to include the product team into the discussion. I leave out the following
I talked with Oliver about it, but this is due to Primer dialogs using a different overlay technique (not z-index). Therefore the dropdown has to be attached to the dialog, resulting in extending the inner scroll area. If I remember right - in other dialogs with this situation, we scroll down on open, so it's visible. But I found that jumping to be equally unsatisfying. So I postponed the topic until ng-select can be changed/replaced. Thanks a lot for the review! |
What are you trying to accomplish?
Screenshots
What approach did you choose and why?
Ticket
https://community.openproject.org/work_packages/48274
https://www.figma.com/file/n0uj7NMxC4Q2nAzMyhdBXF/Export-work-packages-table?type=design&node-id=0-1&mode=design
Merge checklist