Status indicator in gutter #598
Conversation
Tymek
force-pushed
the
master
branch
2 times, most recently
from
9527dfe
to
817e516
Compare
There was a problem hiding this comment.
hey, thanks for the PR! This is one of the highly sought after issues. 🙏
A few suggestions:
- the build failed because you need to update the changelog
- to help accessibility and consider color blind use case, let's use different shapes in addition to colors for icons. you can reference the original indicators as examples.
src/decorations.ts
Outdated
| return window.createTextEditorDecorationType({ | ||
| overviewRulerColor: 'red', | ||
| overviewRulerColor, | ||
| gutterIconPath: context.asAbsolutePath(`images/icons/${icon}.svg`), |
There was a problem hiding this comment.
will this path be a problem in windows? how about this
| gutterIconPath: context.asAbsolutePath(`images/icons/${icon}.svg`), | |
| gutterIconPath: context.asAbsolutePath(path.join('images', 'icons', `${icon}.svg`)), |
There was a problem hiding this comment.
tested with on Win10+VSCodium, clean install. It does this internally I think. won't hurt. I'll fix it
There was a problem hiding this comment.
changed to what is already in repo (see src/Coverage/Formatters/GutterFormatter/index.ts:46)
There was a problem hiding this comment.
actually there is a bug regarding GutterFormatter icon (#585), but it might be a packaging issue and not necessarily the path... I don't have any windows machine, not sure if there is any variation that could cause a path problem... unless we are absolutely sure otherwise it is probably better to be safe...
I actually think we should update GutterFormatter 1. the path formation, 2. consolidate the svg files to the same location as yours.
Pull Request Test Coverage Report for Build 799
💛 - Coveralls |
|
I am thinking about cutting a beta soon... if you can get the new icons in on Monday, I will wait.
hmmm... let's hold off on that, I think the default set that works in both lite/dark mode should be sufficient. |
|
I think selected colors are readable on both dark and light backgrounds. It's not worse than before, so should be enough for now. Customization will make dark/light versions obsolete. SVGs from I opted to use square for skipped tests and circle for not-ran. It looks much better this way. Counterargument could be that Jest itself is using empty circle for skipped tests, so it's not super-consistent. Additional shape is an upgrade nonetheless. GIF in Readme needs an update for 4.0 🙂 |
Tymek
force-pushed
the
master
branch
2 times, most recently
from
2a4e827
to
e177c64
Compare
There was a problem hiding this comment.
the code looks good, here are some other comments:
UX
- the pass and fail looked nice
- skip, with an empty box, is not quite clear and visually a bit "weak", maybe something solid and more symbolic... something like the debug mode "skip-over" icon?
- unknown with a circle outline and identical color as line-number is also a bit confusing and hard to spot... maybe a question mark instead?
oh and I only tested with dark mode, did you check if they look good in lite mode as well?
webpack
I think the current webpack is not picking up any svg file, please see if you can fix it. (this will address #585 too)
GutterFormatter
as I mentioned in an earlier comment, let's consolidate all the svg files in the same directory.
|
devops issues. I will open new pull request with the same changes but from new branch (and remember not to make the same mistake again, when pull bot wiped my master). Link should appear below in a sec |
closes #554