-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Emmet Wrap with Abbreviation History, fixes #40096 #114170
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test this yet, but I wonder how it performs when compared to something simpler like the commit message box history. I think we should aim for something light still and I could imagine after several emmet usages, the list could get long and cluttered making basic emmet usage more cumbersome. I know we cannot use an input box with history from an extension but the spirit remains.
* Both successful (accepted) and non-successful (cancelled) expansion attempts are stored. | ||
* The expansion attempts are stored per extension reload. | ||
*/ | ||
let previousItems: Set<string> = new Set(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be const if you don't recreate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be initialized from previous sessions. e.g. if I press up in the commit message box, i can see commit messages from long ago
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that can go into a separate feature, since firstly, I'd have to look into where to write the abbreviation history to, and secondly, I'd probably need to add a "clear history" command.
previousItems.add(valueToUse); | ||
resolve(valueToUse); | ||
} else { | ||
resolve(''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value is passed to makeChanges, and both undefined and '' would do the same thing in this case.
0376024
to
b52667f
Compare
Just tested -- the quickpick comes with a scrollbar, so it isn't too bad. |
11a683d
to
29fed26
Compare
Added response as comment, also testing this GitHub feature
29fed26
to
a569ac3
Compare
Discussed offline with @sbatten |
This PR fixes #40096 by replacing the inputbox that shows up during
Wrap with Abbreviation
with a quickpick.