-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Added new style settings (color, size, etc.) for easymotion markers #1493
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.
Overall looks good to me! Just a few little things
src/easymotion/easymotion.ts
Outdated
|
||
// Compute font color based on background (remove opacity) | ||
var Color = require('color'); | ||
let backgroundColor = Color.rgb(Configuration.searchHighlightColor).alpha(1.0); |
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.
Can you remove this dependency from package.json since this was the only place it was used
@@ -61,41 +61,41 @@ export class EasyMotion { | |||
var totalSteps = 0; | |||
|
|||
if (length >= keyTable.length) { | |||
var totalRemainder = Math.max(length - keyTable.length, 0); | |||
totalSteps = Math.floor(totalRemainder / keyTable.length); | |||
var totalRemainder = Math.max(length - keyTable.length, 0); |
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 noticed all the formatting was wrong before last week, I imagine you just used the formatter on these files right?
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.
Yeah I think I have my vscode set to auto format on save
src/easymotion/easymotion.ts
Outdated
this.setBackgroundColor = backgroundColor; | ||
} | ||
// Clear cache if the backgroundColor has changed | ||
if (this.setBackgroundColor !== backgroundColor) { |
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.
Not a huge deal but the cached resources will be out of date whenever you change a setting BESIDES the background color, so tuning the colors is a bit complicated. A clean restart of vscode handles this. It might be a good idea to check some other things here and clear the cache in the case of any main changes now that they are all configurable at runtime
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.
yeah I noticed that but didn't realize it was b/c of cache, I'll see if I can fix it
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 just be able to add a few more checks right here
src/easymotion/easymotion.ts
Outdated
if (backgroundColor.light()) { | ||
fontColor = "black"; | ||
} | ||
// var Color = require('color'); |
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.
remove
src/easymotion/easymotion.ts
Outdated
fontColor = keystroke.length > 1 ? "orange" : "red"; | ||
backgroundColor = Color('black'); | ||
} | ||
let fontColor = keystroke.length > 1 ? |
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.
const
src/easymotion/easymotion.ts
Outdated
let fontColor = keystroke.length > 1 ? | ||
Configuration.easymotionMarkerForegroundColorTwoChar | ||
: Configuration.easymotionMarkerForegroundColorOneChar; | ||
let backgroundColor = Configuration.easymotionMarkerBackgroundColor; |
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.
const
I think it should be mostly fixed up now, I tested a bit and it seems to update fine now without restarting. |
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.
One last thing can you update the readme section for easymotion with a brief mention of the settings related to it? Then should be all good!
Awesome @edasaki thanks a lot! You will see it in the next release :) |
Made this for my own issue :P #1481
Here's an example of the settings in use:
I removed the existing easymotion background color option since I feel this encompasses that, but feel free to add it back if you think it's better to keep it!
Here are some examples of what the styling can look like: (p.s. I happened to use white and black for the font colors but they can be anything, like yellow text on dark blue bg or something)