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

monaco: adjust where the monaco menu appears #10909

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Mar 22, 2022

What it does

Fixes: #10907

The pull-request adjusts the monaco menu (quick-input) to appear after the main-menu and not overlay it.

monaco-menu-improvement.mp4

How to test

  1. start the browser application
  2. ensure that the different menus work well (quick-file open, quick-commands, etc.)
  3. start the electron application with the native titlebar style
  4. repeat step 2
  5. start the electron application with the custom titlebar style
  6. repeat step 2
  7. the change should also work nicely with the toolbar toggled

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added monaco issues related to monaco ui/ux issues related to user interface / user experience labels Mar 22, 2022
@vince-fugnitto vince-fugnitto marked this pull request as ready for review March 22, 2022 18:53
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

While the solution is quite clean, I'm wondering whether this is the right approach. When attaching the div to the main panel of Theia, it will be offset from the center of the app. See this:

image

Comment on lines 7 to 12
#quick-input-container {
position: absolute;
top: 0px;
right: 50%;
z-index: 1000000;
}

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering whether we need this change. It doesn't seem to do anything, and removing it doesn't change the behavior.

@vince-fugnitto
Copy link
Member Author

While the solution is quite clean, I'm wondering whether this is the right approach. When attaching the div to the main panel of Theia, it will be offset from the center of the app. See this:

@msujew ahh you're right, I see the odd behavior when I modify the layout. I'm not checking what might be a better choice to append to, but I'm also worried if downstream they modify their shell or layout. Do you have any suggestions?

@msujew
Copy link
Member

msujew commented Mar 23, 2022

Yeah, the downstream issue is difficult. I have two ideas:

  1. Disregard downstream apps that modify their layout and simply append to the html element with the #theia-left-right-split-panel id. That way we always stay in the center of the app with the quick input.
  2. Keep the quick input attached to the body of the document and simply change the top style attribute to consider the top-panel height. I assume we would need to change the --theia-private-menubar-height css variable to 0px if we hide the top panel in any way (electron native mode/compact menu bar mode). It's probably a more complicated approach.
  3. The same as 2. but use add class for the quick input container if the top-panel is visible, which sets top to var(--theia-private-menubar-height) and keeps it at 0 otherwise.

@vince-fugnitto
Copy link
Member Author

@msujew I modified the code slightly to use a fixed position and it seems to have resolved the issue, is there an issue with this approach?

@vince-fugnitto vince-fugnitto marked this pull request as ready for review March 23, 2022 12:48
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Alright, that approach looks good!

The commit adjusts the monaco menu (quick-input) to appear after the
main-menu and not overlay it.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

LGTM. The quick input no longer obscures the menu bar.

@vince-fugnitto vince-fugnitto merged commit 4b04548 into master Mar 28, 2022
@vince-fugnitto vince-fugnitto deleted the vf/monaco-menu branch March 28, 2022 16:08
@github-actions github-actions bot added this to the 1.24.0 milestone Mar 28, 2022
@colin-grant-work
Copy link
Contributor

It looks like appending the QP container to the main area has negative side effects when side panels are expanded:

image

Perhaps we should revert to attaching the QP to the body, but calculate its top value to match the top of main when showing the quick pick?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco ui/ux issues related to user interface / user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

monaco: menus should not overlay the main-menu
3 participants