[#72239] Workflows UX improvement: Select relevant roles and statuses #22325
[#72239] Workflows UX improvement: Select relevant roles and statuses #22325mrmir wants to merge 17 commits intoepic/workflows-ux-quick-winsfrom
Conversation
6548db3 to
24de72b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
…y-type' into feature/72239-workflows-ux-improvement-select-relevant-statuses
Before, the statuses in the list were only those read from the DB. Thus, adding/removing statuses and clicking apply did not update the list before saving the entire form
…y-type' into feature/72239-workflows-ux-improvement-select-relevant-statuses
…ows-ux-improvement-select-relevant-statuses
363121d to
5be719d
Compare
There was a problem hiding this comment.
Nice 👍 🌟
When playing around I noticed some issues:
- The Subheader is part of the scrollable area resulting in something like this:
- The dialog to select statuses is too small. I know that you added the class, but given that the autocompleter will grow very quickly, this is not enough.
-
I somewhow expected that a change in role would change the URL. Currently, when I change the role, and reload (or change the tab), the role selection is lost.
-
In the ticket it says: "When the page is saved with one status completely unused (no checkboxes checked for its row/column), it is removed on save." This is not happening:
| role: @role, | ||
| type: @type, | ||
| tab: @tab, | ||
| dialog_id: dialog_id |
There was a problem hiding this comment.
nitpick:
| dialog_id: dialog_id | |
| dialog_id: |
| dialog.with_additional_details do | ||
| concat(hidden_field_tag(:role_id, @role.id)) | ||
| concat(hidden_field_tag(:tab, @tab)) | ||
| @status_ids.each { |id| concat(hidden_field_tag("status_ids[]", id)) } | ||
| end |
There was a problem hiding this comment.
I think this does not belong here. The dangerDialog is to get users confirmation (or if necessary decide what to do with related objects). These hidden objects should however be part of the original form request.
There was a problem hiding this comment.
I'm not sure how else to pass these params. The reason I added them here was because the status dialog "Apply" is not saving anything to the DB. So in the case where something is removed and the danger dialog is shown, the new info (statuses) needs to be passed through the dialog. Otherwise the params reaching WorkflowsController#confirm_statuses (on dialog "Apply") are lost
| end | ||
| end | ||
|
|
||
| def role_type_statuses |
There was a problem hiding this comment.
Might be a personal preference, but the name could probably be better explaining.. Maybe statuses_for_role_and_type or something similar?
config/locales/en.yml
Outdated
| title: "Statuses" | ||
| label: "Statuses enabled for this type" | ||
| caption: "Add or remove statuses you would like to associate with this type. Removing a status will also delete the workflow associated with it." | ||
| apply: "Apply" |
There was a problem hiding this comment.
We already have a generic "apply" string (button_apply). Is it really necessary to add a new one here?
config/locales/en.yml
Outdated
| caption: "Add or remove statuses you would like to associate with this type. Removing a status will also delete the workflow associated with it." | ||
| apply: "Apply" | ||
| statuses_removal_dialog: | ||
| title: "Remove statuses?" |
There was a problem hiding this comment.
I vaguely remember, that we once decided in the D-Team to follow the pattern of adding the question mark only to the visible title, not the one that is read out by screenreaders. So that would mean:
| title: "Remove statuses?" | |
| title: "Remove statuses" |
| render Primer::OpenProject::SubHeader.new do |subheader| | ||
| if @type && @available_roles.any? | ||
| subheader.with_filter_component do | ||
| render(Primer::Alpha::ActionMenu.new(select_variant: :single)) do |menu| |
There was a problem hiding this comment.
According to the figma comment, this should be a SelectPanel and not an action menu
There was a problem hiding this comment.
Hmm in the WP AC it was either SelectPanel or ActionMenu, and I updated it to be ActionMenu since that made more sense to me. Since it was an either/or, I think this is fine too?
frontend/src/stimulus/controllers/dynamic/admin/workflow-checkbox-state.controller.ts
Show resolved
Hide resolved
This is generally happening for me and there's also a test that covers this case. Is there something specific about the case where it doesn't work for you? |
|
I made changes according to your feedback and left some clarification questions, so please take another look |
Ticket
https://community.openproject.org/work_packages/72239
What approach did you choose and why?
Merge checklist