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

Implementation/53233 replace sidemenu of work package page with rails component #15982

Conversation

HDinger
Copy link
Contributor

@HDinger HDinger commented Jun 26, 2024

ToDo

  • Use Common::SubmenuComponent to render the submenu of the WorkPackages module
  • Follow the same code structure we introduced recently for the other modules
  • Take care that all of the special default queries are correctly highlighted ("all open", "summary", etc)
  • Get tests green
  • Remove op-view-select and reduce the op-static-queries.service to the functionality of gettting the name of the query but not building them any more.

https://community.openproject.org/projects/openproject/work_packages/53233/activity

By chance, this also fixes https://community.openproject.org/projects/openproject/work_packages/52954/activity and https://community.openproject.org/projects/openproject/work_packages/52759/activity

@HDinger HDinger added this to the 14.3.x milestone Jun 26, 2024
@HDinger HDinger force-pushed the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch 7 times, most recently from 153a758 to c894015 Compare July 1, 2024 07:08
@HDinger HDinger marked this pull request as ready for review July 1, 2024 08:40
@HDinger HDinger force-pushed the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch from e626888 to a895a64 Compare July 1, 2024 09:49
@HDinger HDinger requested a review from oliverguenther July 1, 2024 11:20
@cbliard cbliard assigned cbliard and unassigned cbliard Jul 1, 2024
@cbliard cbliard self-requested a review July 1, 2024 13:04
Copy link
Member

@cbliard cbliard left a comment

Choose a reason for hiding this comment

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

I did not look at the code yet.

I found the following issues with the work package sidebar menu:

  • When displaying the menu when clicking on the arrow next to "Work packages", multiple items are selected/highlighted, but none should be highlighted

    1. Go to a project > Overview
    2. In the side bar menu, click the arrow next to "Work packages"
    • => expected: nothing highlighted (can be checked on qa.openproject-stage.com)
    • => actual: "Project plan" and "All open" are highlighted
    • Note: The one on "Project plan" seems to vanish when focusing somewhere else, like clicking in the search bar
    • image
  • When modifying filters, the selected menu item is still hightlighted. But I'm not sure if that's wanted or if it's a regression compared to before:

    1. Go to project > Work packages > All open
    2. Change filter status from "status = open" to "status = closed".
    • => expected: nothing highlighted as the filters have been changed and a little "save" icon is displayed (at least, that's the behavior on qa.openproject-stage.com)
    • => actual: the "All open" menu item remains highlighted.
    • Note: the behavior on stage is not ideal either as the highlighted menu item is easily lost, even without modifying anything on the filters (for instance on stage, click "All open", then "Latest activity", then click in the blank area below the work package table => menu item is unselected and the little save icon is shown, but it should not be).

And what works well 👍 :

  • renaming a view => renamed in menu
  • saving as a view => a new item is created in menu
  • deleting a view => view is removed
  • changing visibility => collapsible sections appear and disappear
  • using another user with restricted access / shared wp access => work well too

@HDinger HDinger force-pushed the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch from a895a64 to ee37ae0 Compare July 3, 2024 11:25
@HDinger
Copy link
Contributor Author

HDinger commented Jul 4, 2024

Hi @cbliard

thanks for your feedback! I talked with the product team about the requirements regarding your second point:

When modifying filters, the selected menu item is still hightlighted. But I'm not sure if that's wanted or if it's a regression compared to before

This was the answer we agreed upon:

it would be logical to keep the selection and change it only if the user saves the changed view with a different name.
That way, the user still knows which view he's "on" even though they might have modified it.

So I will stick with the current behavior.


Regarding the first point:

When displaying the menu when clicking on the arrow next to "Work packages", multiple items are selected/highlighted, but none should be highlighted

I fixed the wrong highlighting of the "All open" query. The other query is indeed highlighted because by default, the first element is focused. However, for me, this happens only when navigating via keyboard which makes sense imho. So I tend to keep that behaviour. What do you think?

@HDinger HDinger force-pushed the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch from e8fbe14 to a609931 Compare July 4, 2024 08:39
@HDinger HDinger requested a review from cbliard July 4, 2024 09:59
@cbliard
Copy link
Member

cbliard commented Jul 4, 2024

Regarding the first point:

When displaying the menu when clicking on the arrow next to "Work packages", multiple items are selected/highlighted, but none should be highlighted

I fixed the wrong highlighting of the "All open" query. The other query is indeed highlighted because by default, the first element is focused. However, for me, this happens only when navigating via keyboard which makes sense imho. So I tend to keep that behaviour. What do you think?

For keyboard navigation this is fine.
It happens with mouse in a very specific scenario: click arrow, go back, click arrow, and first item is selected.
But it's very specific, it should not block merging.

Kapture.2024-07-04.at.14.52.59.webm

@cbliard
Copy link
Member

cbliard commented Jul 4, 2024

Hi @HDinger,

I found another issue: when a query is saved, or when it is selected, then "All open" is also selected.

Kapture.2024-07-04.at.15.00.02.webm

And for the rest, it works very well.

@HDinger HDinger force-pushed the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch from a609931 to 414101c Compare July 5, 2024 06:36
@HDinger HDinger merged commit c5fe2d5 into dev Jul 5, 2024
11 checks passed
@HDinger HDinger deleted the implementation/53233-replace-sidemenu-of-work-package-page-with-rails-component branch July 5, 2024 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

2 participants