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

fix monaco suggest list highlight color overlap in light theme #12317

Conversation

venciallee
Copy link
Contributor

@venciallee venciallee commented Mar 16, 2023

What it does

Closes: #12048.

How to test

  1. Suggest list highlight should work as expected.
  2. Following steps from Unreadable content assist selection #12048 and no highlight color overlap

Review checklist

Reminder for reviewers

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.

Thank you for your contribution 👍

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

packages/monaco/src/browser/style/index.css Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added theming issues related to theming monaco issues related to monaco labels Mar 16, 2023
@tsmaeder
Copy link
Contributor

tsmaeder commented Apr 3, 2023

@vince-fugnitto, @venciallee from reading through the comments, I'm not sure if we're going to go forward with this change.

@vince-fugnitto
Copy link
Member

@vince-fugnitto, @venciallee from reading through the comments, I'm not sure if we're going to go forward with this change.

Right, the pull-request cannot be merged as is as it introduces regressions to other menus and stylings.
@venciallee are you still interested in contributing a fix?

@venciallee
Copy link
Contributor Author

venciallee commented Apr 12, 2023

Yep, Let me fix it in the next few days. Sorry for replying so late.

@venciallee venciallee force-pushed the bugfix/monaco-suggest-highlight-color branch 2 times, most recently from a480355 to bdc7a9d Compare April 18, 2023 05:29
@venciallee
Copy link
Contributor Author

Yep, Let me fix it in the next few days. Sorry for replying so late.

Done, Already work for me in light/dark/red theme, please review again @vince-fugnitto

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.

I think it's closer, but there is still a regression with the matching highlight:

before (master);

after-fuzzy.mp4

after (this pr):

before-fuzzy.mp4

The blue highlight from the theme when it matches the query is not being applied with the recent changes.

@vince-fugnitto vince-fugnitto modified the milestone: 1.37.0 Apr 27, 2023
@venciallee
Copy link
Contributor Author

I think it's closer, but there is still a regression with the matching highlight:

before (master);

after-fuzzy.mp4
after (this pr):

before-fuzzy.mp4
The blue highlight from the theme when it matches the query is not being applied with the recent changes.

Thanks for your review.

I test it in master branch(commit 877f962), and .quick-input-list .highlight is light blue. Did i missing something?

Screen.Recording.2023-04-29.at.16.39.41.mov

Here's the .quick-input-list .highlight

1682757530845

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.

I re-reviewed and the changes look good to me, I was confused by the fact that I thought master handled the fuzzy highlighting color from themes better but this pr is going in the right direction.

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

@venciallee
Copy link
Contributor Author

I re-reviewed and the changes look good to me, I was confused by the fact that I thought master handled the fuzzy highlighting color from themes better but this pr is going in the right direction.

In order to accept your changes please be sure to sure to sign the eclipse contributor agreement (eca) with the same email as your authorship.

Thanks, Already signed ECA with account venciallee@gmail.com

@vince-fugnitto
Copy link
Member

Thanks, Already signed ECA with account venciallee@gmail.com

@venciallee sound good, the check fails because the authorship and your eca email do not match:

From bdc7a9d489b35191344cc9f882ac85c8ea1f86c2 Mon Sep 17 00:00:00 2001
From: liwenchao <liwenchao.24@bytedance.com>
Date: Tue, 18 Apr 2023 13:28:38 +0800
Subject: [PATCH]  fix monaco suggest list highlight color overlap in light
 theme

Signed-off-by: vencial lee <venciallee@gmail.com>

...

One being the bytedance email and the other your gmail.

@venciallee venciallee force-pushed the bugfix/monaco-suggest-highlight-color branch from bdc7a9d to b7389ac Compare May 4, 2023 03:10
@venciallee
Copy link
Contributor Author

venciallee commented May 4, 2023

Thanks, Already signed ECA with account venciallee@gmail.com

@venciallee sound good, the check fails because the authorship and your eca email do not match:

From bdc7a9d489b35191344cc9f882ac85c8ea1f86c2 Mon Sep 17 00:00:00 2001
From: liwenchao <liwenchao.24@bytedance.com>
Date: Tue, 18 Apr 2023 13:28:38 +0800
Subject: [PATCH]  fix monaco suggest list highlight color overlap in light
 theme

Signed-off-by: vencial lee <venciallee@gmail.com>

...

One being the bytedance email and the other your gmail.

Thanks for remind. Already commit with "venciallee@gmail.com" account.

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.

@venciallee the behavior and changes look good to me, do you mind rebasing the pull-request (I'm hoping it would fix some of the CI failures we are noticing).

Signed-off-by: venciallee <venciallee@gmail.com>
@venciallee venciallee force-pushed the bugfix/monaco-suggest-highlight-color branch from b7389ac to 30d270c Compare May 13, 2023 14:00
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.

LGTM 👍 Thank you for rebasing, the CI now successfully passes

@vince-fugnitto vince-fugnitto merged commit 7f1d6a8 into eclipse-theia:master May 15, 2023
@vince-fugnitto vince-fugnitto added this to the 1.38.0 milestone May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
monaco issues related to monaco theming issues related to theming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unreadable content assist selection
3 participants