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.

Sign up for GitHub

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

Jump to bottom

monaco: quick input regression fix #12239

Merged
merged 1 commit into from
May 31, 2023
Merged

monaco: quick input regression fix #12239

merged 1 commit into from
May 31, 2023

Conversation

FernandoAscencio
Copy link
Contributor

@FernandoAscencio FernandoAscencio commented Feb 28, 2023 •
edited
Loading

What it does

Fixes a regression introduced in #12095

How to test

Currently the commit in #12095 introduced an regression with some menus did not display properly.

12095Regression

To test, you can run the Start Debugging while on Attach by Process ID. Then check that the display is fixed and no other displays have broken.

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added monaco issues related to monaco ui/ux issues related to user interface / user experience labels Feb 28, 2023
colin-grant-work
colin-grant-work approved these changes Mar 1, 2023
View reviewed changes
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.

A definite improvement 👍

@gbodeen gbodeen mentioned this pull request Mar 1, 2023
Closed
1 task
@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from 68fe499 to 885bd0d Compare March 2, 2023 18:09
vince-fugnitto
vince-fugnitto reviewed Mar 2, 2023
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

There are issues when we have the seti icon-theme set and attempt to quick-open files:

image

@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from 885bd0d to c8e7032 Compare March 2, 2023 20:00
@FernandoAscencio
Copy link
Contributor Author

This should address the issues brought in #12247

QIRegression2

@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch 3 times, most recently from 0d7b6ff to b2aa32a Compare March 3, 2023 18:55
colin-grant-work
colin-grant-work approved these changes Mar 7, 2023
View reviewed changes
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.

These changes appear to address the problems in the three problem areas that have been identified in testing this and related changes: long paths, multi-line picks (attach by process ID), and check marks for toggled commands.

packages/monaco/src/browser/style/index.css Outdated Show resolved Hide resolved
@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from b6d7901 to 94eaf02 Compare March 8, 2023 16:34
@gbodeen
Copy link
Contributor

gbodeen commented Mar 13, 2023

The text alignment is fixed ✅
The icon sizing is still mismatched:
image

@gbodeen
Copy link
Contributor

gbodeen commented Mar 13, 2023

This doesn't necessarily have to be part of this MR (though a fix was included in !12247) -- if not included, we should open an issue for it.

In light themes, the path is no longer easily distinguished from the filename as in VSCode:

image

@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from 94eaf02 to ee07fb0 Compare March 21, 2023 16:22
@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from ee07fb0 to 005c21b Compare March 29, 2023 17:14
@tsmaeder
Copy link
Contributor

@colin-grant-work do we want to merge this one ahead of the release?

@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from ee1a94d to 225d284 Compare March 31, 2023 15:26
@tsmaeder
Copy link
Contributor

@gbodeen @vince-fugnitto what's the state on this one? We have a 👍 , but it's not merged.

vince-fugnitto
vince-fugnitto reviewed May 31, 2023
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

@FernandoAscencio the changes look good to me, do you mind updating this value to 0.9 similarly to VS Code. It should help better differentiate between the primary label and item descriptions (for example: when displaying multi-root paths for the quick-file open):

theia/packages/monaco/src/browser/style/index.css

Line 178 in e9cd687

font-size: calc(var(--theia-ui-font-size1) * 0.95) !important;

@FernandoAscencio
monaco: quick input regression fix
a550ba7 
This commit is the first attempt to fix the regression introduced in
 eclipse-theia#12095

Signed-Off-By: FernandoAscencio <fernando.ascencio.cama@ericsson.com>
@FernandoAscencio FernandoAscencio force-pushed the fa/QuickInputRegressionFix branch from 225d284 to a550ba7 Compare May 31, 2023 16:57
@FernandoAscencio
Copy link
Contributor Author

@vince-fugnitto
Changed 0.95 to 0.9.

vince-fugnitto
vince-fugnitto approved these changes May 31, 2023
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes are a nice improvement 👍 and fix the regressions.

@vince-fugnitto vince-fugnitto merged commit fe083c6 into eclipse-theia:master May 31, 2023
@vince-fugnitto vince-fugnitto deleted the fa/QuickInputRegressionFix branch May 31, 2023 18:54
@vince-fugnitto vince-fugnitto added this to the 1.39.0 milestone Jun 29, 2023
@vince-fugnitto vince-fugnitto mentioned this pull request Aug 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@colin-grant-work colin-grant-work Awaiting requested review from colin-grant-work

@gbodeen gbodeen Awaiting requested review from gbodeen

Assignees
No one assigned
Labels
monaco issues related to monaco ui/ux issues related to user interface / user experience
Projects
PR Backlog
Archived in project
Milestone
1.39.0
Development

Successfully merging this pull request may close these issues.
5 participants
@FernandoAscencio @gbodeen @tsmaeder @vince-fugnitto @colin-grant-work