$(mark-github) in notification

[hediet]

Happens during GitHub sign in.

Not sure who provides the GitHub auth provider.

[TylerLeonhardt]

@bpasero (you seem to be the owner but maybe it's actually @jrieken?) I tried moving from a status bar item that handled progress to the withProgress API. This was the result of that:

1. spinny icon and GitHub icon are too close together
2. $(mark-github) gets displayed in notification.

I don't think I have any control over this since I'm just using the withProgress API but let me know if there's a way.

If not, I can remove the $(mark-github) from the title for now.

[bpasero]

I think we made the decision at one point to not allow icons in notifications to avoid abuse.

spinny icon and GitHub icon are too close together

You mean in the status bar?

[TylerLeonhardt]

I think we made the decision at one point to not allow icons in notifications to avoid abuse.

In this case, can you strip out any icons in notifications?

You mean in the status bar?

Yeah. They're like right next to each other in the screenshot

[bpasero]

Yeah not sure why we don't do that.

As for icons in the status bar: not sure what to do here, if we just add margins we might break it for existing extensions that have worked around it.

[TylerLeonhardt]

not sure what to do here, if we just add margins we might break it for existing extensions that have worked around it.

@bpasero So one of the ways I tried to work around this was adding a space in front of the icon:

vscode/extensions/github-authentication/src/githubServer.ts

Line 181 in 39c6779

title: localize('signingIn', " $(mark-github) Signing in to github.com..."),

but I didn't see any noticeable change. Locally I tried adding like 100 spaces and still get:

[bpasero]

I have extracted the status bar issue into #145852

[jrieken]

In this case, can you strip out any icons in notifications?

The API doc should spell out when a label supports theme icons, like here.

@bpasero I not sure that simply stripping out $(abc) is valid. This means that messages have changed their behaviour and that escaping is needed - despite us not supporting theme icon rendering here

[bpasero]

Yeah thanks for reopen, the fix was not clever. Instead I am now stripping the icon only for this particular case, where a progress starts in the status bar where icons are supported and then moves to be a notification by the user clicking it. Since this is not something the extension can control, we have to strip the codicons there.

But we should not strip them from all notification messages 👍

[bpasero]

@TylerLeonhardt to clarify:

• use ProgressLocation.Window and you can use codicons, the progress will show in the status bar and when shown in notifications have the icon removed
• use ProgressLocation.Notification and you cannot use codicons, so please remove from message

I will update our JSDoc too to make that clear.

[TylerLeonhardt]

@bpasero I only use ProgressLocation.Window so I don't think I need to do anything here.

vscode/extensions/github-authentication/src/githubServer.ts

Line 181 in 39c6779

title: localize('signingIn', " $(mark-github) Signing in to github.com..."),

Thanks for jumping on this!

[bpasero]

Yeah you are all set.