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: fix quick-input-list styling #10923

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Conversation

vince-fugnitto
Copy link
Member

What it does

Fixes: #10922 (comment).

The pull-request removes superfluous styling from #10544 which caused issues in monaco after the latest upgrade:

  1. fixes the peek widget styling

image

  1. fixes the styling of codicons as tail icons in the quick-input
run-task.mp4

How to test

  1. confirm that the peek widget styling is correct - (ex: peek definition)
  2. confirm that codicons in the quick-input menu work well (ex: run task...)

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto [email protected]

@vince-fugnitto vince-fugnitto added monaco issues related to monaco ui/ux issues related to user interface / user experience labels Mar 24, 2022
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.

I noticed that VSCode applies the padding of the .action-item class differently than we do. The action-item padding is actually applied to the action-label class, so we apply padding to codicons inside of monaco twice. We should probably add another rule to account for this:

.action-item .action-label {
  padding: none;
}
/* OR (probably better) */
.monaco-editor .action-item {
  padding: none;
}

Notice how the icon has no rounded border due to the additional padding:

image

@vince-fugnitto
Copy link
Member Author

@msujew thanks! It seems like the hover of the codicons in the quick-input menu have bigger issues so I'll investigate what is needed there.

@vince-fugnitto
Copy link
Member Author

@msujew I'm struggling a bit with the css in the quick-input for the codicons (action-item), I'm wondering if you would have more luck especially since you nicely improved our tabbars with the new styling.

@msujew
Copy link
Member

msujew commented Mar 25, 2022

@vince-fugnitto I can see why you're struggling, using action-item to implement the hover behavior was incorrect in the first place, so I guess that's on me :)

Here's a patch you can apply to fix the behavior. It changes our usage of action-item to be action-label. That automatically fixes the issues in monaco.

@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-styling-fix branch from c4734c9 to e415dc7 Compare March 25, 2022 12:07
@vince-fugnitto
Copy link
Member Author

@msujew thank you for your help! It definitely looks much better, I had confirmed the peek, toolbar, global toolbar and the quick-input menus with your changes.

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.

@vince-fugnitto You're welcome :)

I noticed a minor issue with codicons in the quick-input. Current:

image

How it should look like:

image

Notice the top and left padding which is set to 0px due to this css rule. I think after removing it, there's another, additional rule needed:

.quick-input-list .quick-input-list-entry-action-bar .action-label.codicon {
  padding: 2px; /* monaco's own css rules use 0px 2px 2px 2px here which removes the left padding */
}

Everything else looks good 👍

@vince-fugnitto
Copy link
Member Author

vince-fugnitto commented Mar 25, 2022

@msujew thanks, did you notice that the top-padding for the hover is still off after the changes?

test - added a brighter background to test:

codicon-padding.mp4

@msujew
Copy link
Member

msujew commented Mar 25, 2022

@vince-fugnitto Yeah, seems like the monaco rule has higher precedence. I tested it using this rule and this seems to work:

.quick-input-list .quick-input-list-entry-action-bar .action-label.codicon {
  color: var(--theia-foreground) !important;
  padding: 2px !important;
}

And replaced this rule.

@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-styling-fix branch from e415dc7 to 6b3b187 Compare March 28, 2022 12:35
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.

Thanks, looks good to me!

  • Quick input items (Task: Run task..., Open recent workspace)+
  • Quick peek window
  • The other tab bar icons still work correctly

.theia/tasks.json Outdated Show resolved Hide resolved
The commit removes superfluous styling which caused issues in monaco:
1. fixes the peek widget styling
2. fixes the styling of codicons as tail icons in the quick-input

Signed-off-by: vince-fugnitto <[email protected]>
Co-authored-by: Mark Sujew <[email protected]>
@vince-fugnitto vince-fugnitto force-pushed the vf/monaco-styling-fix branch from 6b3b187 to 905227c Compare March 28, 2022 13:11
@vince-fugnitto vince-fugnitto merged commit 51cbe06 into master Mar 29, 2022
@vince-fugnitto vince-fugnitto deleted the vf/monaco-styling-fix branch March 29, 2022 11:46
@github-actions github-actions bot added this to the 1.24.0 milestone Mar 29, 2022
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: adjust styling of the peek widget
2 participants