[63889] Move toggle button to the sidebar #19170
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HDinger
force-pushed
the
implementation/63889-move-toggle-button-to-the-sidebar
branch
9 times, most recently
from
d7fa135 to
5552e88
Compare
myabc
reviewed
Jun 18, 2025
app/components/open_project/common/main_menu_toggler_component.rb
Outdated
Show resolved
Hide resolved
myabc
requested changes
Jun 18, 2025
There was a problem hiding this comment.
A good start, but there are a few things I think we should address:
Close/Collapse button
Screen.Recording.2025-06-18.at.11.52.07.mov
- the close/collapse button renders underneath some form components, e.g. rich text areas/CKeditor instances.
- I think the close/collapse button would also benefit from being opaque (i.e.
scheme: secondary). We might even consider a small drop shadow:
Alternatively, we could hide the close/collapse button on scroll?
Project select trigger button
The trigger button renders a title tag - it would be good to use the tooltip here - to be consistent with the open/expand button directly to the right.
N.B. calling ToolTipElement from Angular components should be feasible once this upstream fix is merged: primer/view_components#3558
Misc
- I've left various inline comments about being more consistent with naming. ✅
- Just as with my comment here, shouldn't we use try to use the Primer scale and CSS variables rather than hard-coding px values?
- I've been trying to get in the habit of writing component specs. While admittedly they're a bit of a pain at first, it would be good to get our test coverage up. They are orders of magnitude faster than feature specs when we just want to verify that we're rendering the right HTML (server-side at least) rather than simulating full user interactions. ✅
app/components/open_project/common/main_menu_toggler_component.rb
Outdated
Show resolved
Hide resolved
app/components/open_project/common/main_menu_toggler_component.rb
Outdated
Show resolved
Hide resolved
app/components/open_project/common/main_menu_toggler_component.sass
Outdated
Show resolved
Hide resolved
app/components/open_project/common/main_menu_toggler_component.rb
Outdated
Show resolved
Hide resolved
frontend/src/app/shared/components/resizer/resizer/main-menu-resizer.component.ts
Outdated
Show resolved
Hide resolved
app/components/open_project/common/main_menu_toggler_component.rb
Outdated
Show resolved
Hide resolved
myabc
reviewed
Jun 18, 2025
Comment on lines
52
to
54
| def scheme | ||
| :invisible | ||
| end |
There was a problem hiding this comment.
Per my main review comment:
Suggested change
| def scheme | |
| :invisible | |
| end | |
| def scheme | |
| @opened ? :invisible : :secondary | |
| end |
myabc
reviewed
Jun 18, 2025
| #menu-toggler--closer | ||
| position: absolute | ||
| top: 18px | ||
| left: 11px |
There was a problem hiding this comment.
Per main review comment, something like this?
Suggested change
| left: 11px | |
| left: 11px | |
| z-index: 5000 | |
| filter: drop-shadow(var(--bgColor-default) 2px 2px 2px) |
HDinger
force-pushed
the
63888-move-project-selector-to-the-sidebar
branch
from
5240242 to
7dde84b
Compare
HDinger
force-pushed
the
implementation/63889-move-toggle-button-to-the-sidebar
branch
2 times, most recently
from
fef9fd4 to
c0c8dba
Compare
HDinger
force-pushed
the
63888-move-project-selector-to-the-sidebar
branch
from
7dde84b to
15ececb
Compare
HDinger
force-pushed
the
implementation/63889-move-toggle-button-to-the-sidebar
branch
from
c0c8dba to
17589d9
Compare
HDinger
force-pushed
the
63888-move-project-selector-to-the-sidebar
branch
from
15ececb to
a00b1ac
Compare
…bles. Instead we use the Primer behaviour for ActionLists
HDinger
force-pushed
the
implementation/63889-move-toggle-button-to-the-sidebar
branch
2 times, most recently
from
9181511 to
08206e0
Compare
…ed/expanded Clean-up sass code Avoid that the type selector in the WP from is scrolled two far when being activated. Currently, it is scrolled twice (one time when activating it and once again when selecting an item). Thus the dropdown overlaps the actual input Move logo to the left side Make naming consistent for MainMenuToggleComponent and add a component test Let menu toggle button scroll with the rest of the page Move modules menu to the left and show it on mobile
HDinger
force-pushed
the
implementation/63889-move-toggle-button-to-the-sidebar
branch
from
08206e0 to
cc90002
Compare
|
Hi @myabc with the product team, we decided for now to let the button scroll on mobile together with the content. We will further improve on this with https://community.openproject.org/wp/65118 |
The user itself doesn't update when a new attribute help text or role is added to them
oliverguenther
approved these changes
Jun 30, 2025
…t the autocompleter dropdown overlaps the input
HDinger
merged commit 20446d2
into
63888-move-project-selector-to-the-sidebar
11 of 13 checks passed
HDinger
deleted the
implementation/63889-move-toggle-button-to-the-sidebar
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Ticket
https://community.openproject.org/wp/63889
https://community.openproject.org/wp/64847
https://community.openproject.org/wp/64832
What are you trying to accomplish?
Screenshots