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

Set gutter colour with CSS #436

Merged
merged 2 commits into from
Mar 10, 2024
Merged

Conversation

package.json Show resolved Hide resolved
@verhovsky verhovsky marked this pull request as draft January 13, 2024 22:08
@verhovsky verhovsky changed the title Configure gutter colour the same way as lines Set gutter colour with CSS Jan 13, 2024
@verhovsky verhovsky force-pushed the config-gutters branch 2 times, most recently from d065906 to c897485 Compare January 13, 2024 23:26
@verhovsky verhovsky marked this pull request as ready for review January 13, 2024 23:26
src/extension/config.ts Fixed Show resolved Hide resolved
src/extension/config.ts Fixed Show fixed Hide fixed
src/extension/config.ts Fixed Show fixed Hide fixed
src/extension/config.ts Fixed Show fixed Hide fixed
src/extension/config.ts Fixed Show fixed Hide fixed
@ryanluker
Copy link
Owner

@verhovsky Thanks for the PR, this will be sweet to have and is much better than the manual icon route.
I do wonder, though, how we will go about releasing this without disrupting the backwards compatibility of the old solution?

I definitely think this should be the new way moving forward, but I wonder if we should leave in the old values (but maybe add deprecated to the description in the packag.json, with a mention of them going away in 3.0?

In the meantime, the extension could use an adaptor pattern to allow us to move forward with the better approach and still provide support for the old file method so we can release this in 2.x? (which will be much sooner than a hypothetical 3.0 heh).

return "";
}

const svg = '<svg width="32" height="48" viewPort="0 0 32 48" xmlns="http://www.w3.org/2000/svg"><polygon points="16,0 32,0 32,48 16,48" fill="' + colour + '"/></svg>';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the tests will also need to be adjusted to support this (or if we support the old method and the new, then we could add a new test to cover this).
https://github.com/ryanluker/vscode-coverage-gutters/actions/runs/7951747196/job/21742231483?pr=436#step:6:128

@verhovsky
Copy link
Contributor Author

verhovsky commented Feb 19, 2024

This would only matter to people that have a different gutter color than the line highlight color and people that only set a gutter color and no line color. I can't imagine anyone going through the trouble of generating an image file just to change the color of a gutter in their code editor, surely not many people use this setting.

@ryanluker
Copy link
Owner

This would only matter to people that have a different gutter color than the line highlight color and people that only set a gutter color and no line color. I can't imagine anyone going through the trouble of generating an image file just to change the color of a gutter in their code editor, surely not many people use this setting.

Alright I agree with that logic, makes sense!
We can also have an addition in extension marketplace readme mentioning a migration path.

const svg = '<svg width="32" height="48" viewPort="0 0 32 48" xmlns="http://www.w3.org/2000/svg"><polygon points="16,0 32,0 32,48 16,48" fill="' + colour + '"/></svg>';

const icon = 'data:image/svg+xml;base64,' + Buffer.from(svg).toString('base64');
return Uri.parse(icon);
Copy link
Owner

@ryanluker ryanluker Feb 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional Logging: This is looking good, I do wonder though if we should add some logging to assist with if the make icon process fails in some way 🤔. (maybe lower down where the calls to this new inline are)

    const outputChannel = vscode.window.createOutputChannel("coverage-gutters");
    const configStore = new Config(context);

expect((options.dark as any).gutterIconPath).to.equal(iconPathDark);
expect((options.light as any).gutterIconPath).to.include("./app_images/");
expect(((options.dark as any).gutterIconPath as any).path).to.be.a('string').and.match(preamble).and.satisfy((icn: string) => atob(icn.replace(preamble, '')).includes(highlightdark));
expect(((options.light as any).gutterIconPath as any).path).to.be.a('string').and.match(preamble).and.satisfy((icn: string) => atob(icn.replace(preamble, '')).includes(highlightlight));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Props 👍🏻 : Cool test, thanks for adding this!

✔ Should set the gutter icon colour to the provided value if set @Unit

Copy link
Owner

@ryanluker ryanluker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet feature addition @verhovsky , thanks for contributing!

@ryanluker ryanluker linked an issue Feb 25, 2024 that may be closed by this pull request
@ryanluker ryanluker added this to the 2.12.0 milestone Feb 25, 2024
@ryanluker ryanluker merged commit 78c85ad into ryanluker:master Mar 10, 2024
5 checks passed
@ryanluker
Copy link
Owner

@verhovsky FYI this is going out with today's release. I was just doing the QA as a double check, and it was working nicely locally for me! Thanks again for the contribution.
image

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

Successfully merging this pull request may close these issues.

Don't highlight tested lines
2 participants