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

Fix contributed profile icons #182615

Merged
merged 2 commits into from
May 19, 2023
Merged

Fix contributed profile icons #182615

merged 2 commits into from
May 19, 2023

Conversation

tisilent
Copy link
Contributor

@tisilent tisilent commented May 16, 2023

Fix #182303 .

The little bug has returned.
企业微信截图_16842321008527

@Tyriar Tyriar added this to the May 2023 milestone May 19, 2023
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks @tisilent! I made the fix a little more generic so it works with all extensions, thanks for the investigation

@Tyriar Tyriar changed the title fix #182303 Fix contributed profile icons May 19, 2023
@Tyriar Tyriar enabled auto-merge May 19, 2023 17:41
@Tyriar Tyriar merged commit bce946e into microsoft:main May 19, 2023
5 checks passed
@tisilent tisilent deleted the #182303 branch May 20, 2023 09:48
@tisilent
Copy link
Contributor Author

@Tyriar You better 👍 ,I learned.

@tisilent
Copy link
Contributor Author

Wait..There are still some issues.

@Tyriar
Copy link
Member

Tyriar commented May 20, 2023

@tisilent oh? It was working when I tested it out?

@tisilent tisilent restored the #182303 branch May 22, 2023 01:46
this.shellLaunchConfig.color = defaultProfile.color;
// Only use default icon and color if they are undefined in the SLC
this.shellLaunchConfig.icon ??= defaultProfile.icon;
this.shellLaunchConfig.color ??= defaultProfile.color;
Copy link
Contributor Author

@tisilent tisilent May 22, 2023

Choose a reason for hiding this comment

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

@Tyriar The 'New Terminal' action requires default profile.

Copy link
Contributor Author

@tisilent tisilent May 22, 2023

Choose a reason for hiding this comment

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

Issues:
企业微信截图_1684722768662
'New Terminal' using terminal icon, pwsh icon should be used.

After modify, the test is working:
// Only use default icon and color if they are undefined in the SLC if (!this.shellLaunchConfig.isExtensionOwnedTerminal) { this.shellLaunchConfig.icon = defaultProfile.icon; this.shellLaunchConfig.color = defaultProfile.color; } else { this.shellLaunchConfig.icon ??= defaultProfile.icon; this.shellLaunchConfig.color ??= defaultProfile.color; }

I'm not sure if there are better property to distinguish. 😖

Tyriar added a commit that referenced this pull request May 22, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contributed icon in JS debug terminal stopped working
3 participants