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 the type and initial value of 'editor.quickSuggestions', fix #5227 #5228 #5883

Closed
wants to merge 2 commits into from

Conversation

BroKun
Copy link
Member

@BroKun BroKun commented Aug 7, 2019

Signed-off-by: Brokun brokun0128@gmail.com

What it does

Fix the type and initial value of 'editor.quickSuggestions', suggestions won't be seen by default in string and comment.

How to test

Modifying preferences, we can now control whether suggestions should be displayed in comments and strings.

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.

@BroKun I tested against both issues #5227 and #5228
Is this the expected behavior by default?

#5227: annoying auto-suggestion when writing comments

Screen Shot 2019-08-07 at 1 13 46 PM

#5228: incorrect auto-suggestion when writing strings

Screen Shot 2019-08-07 at 1 16 11 PM

@akosyakov akosyakov added the monaco issues related to monaco label Aug 8, 2019
@BroKun
Copy link
Member Author

BroKun commented Aug 8, 2019

@vince-fugnitto In addition to the quickSuggestion, there are triggerCharacters to trigger suggestions, as is the case in your GIF. In this case, we are still not the same as VS Code, I will look at how to deal with it.

@akosyakov
Copy link
Member

@vince-fugnitto @BroKun is there something left to do to land this PR?

@vince-fugnitto
Copy link
Member

@vince-fugnitto @BroKun is there something left to do to land this PR?

When testing it wasn't working for me (see GIFs), unless I turned off editor.suggestOnTriggerCharacters which I don't think is a viable workaround.

@BroKun
Copy link
Member Author

BroKun commented Aug 18, 2019

@vince-fugnitto @BroKun is there something left to do to land this PR?

I think this work is now compelete, but I noticed that there is a commit that is duplicated with another #5931 . Should I handle some my commit based on another PR ?

@akosyakov
Copy link
Member

@BroKun let's wait till #5931 is merged, then you can rebase and then we merge it
@vince-fugnitto Could you give it a try please?

@akosyakov
Copy link
Member

@BroKun Could you rebase it please?

@BroKun
Copy link
Member Author

BroKun commented Sep 16, 2019

@akosyakov After monaco upgraded to 0.17.0, these configurations have all been migrated. Let's close this PR.

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

Successfully merging this pull request may close these issues.

3 participants