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

Browse themes to install in the color theme picker #136371

Merged
merged 4 commits into from Nov 4, 2021
Merged

Conversation

aeschli
Copy link
Contributor

@aeschli aeschli commented Nov 3, 2021

No description provided.

@aeschli aeschli requested a review from sandy081 November 3, 2021 17:16
@aeschli aeschli self-assigned this Nov 3, 2021
@aeschli aeschli added this to the November 2021 milestone Nov 3, 2021
@aeschli aeschli added the themes Color theme issues label Nov 3, 2021
@aeschli
Copy link
Contributor Author

aeschli commented Nov 3, 2021

@sandy081 FYI the changes to src/vs/workbench/services/extensionResourceLoader/electron-sandbox/extensionResourceLoaderService.ts as discussed a while ago.

Copy link
Member

@sandy081 sandy081 left a comment

Choose a reason for hiding this comment

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

Just reviewed changes in extensionResourceLoadedService. Because, I think I am not an expert in other files. Otherwise, please let me know if you want feedback in particular.

private _getExtensionResourceAuthority(uri: URI): string | undefined {
const index = uri.authority.indexOf('.');
return index !== -1 ? uri.authority.substring(index + 1) : undefined;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we somehow reuse this in both services (browser & electron-sandbox) versions, instead of duplicating?

Copy link
Contributor Author

@aeschli aeschli Nov 4, 2021

Choose a reason for hiding this comment

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

Yes. I would make a proposal for that in a follow-up PR, if that's ok. I think it's probably best done in a abstract base class for the ExtensionResourceLoaderService

@aeschli
Copy link
Contributor Author

aeschli commented Nov 4, 2021

Just reviewed changes in extensionResourceLoadedService. Because, I think I am not an expert in other files. Otherwise, please let me know if you want feedback in particular.

Thanks, that's perfect (and the intent)

@aeschli aeschli merged commit 43fa8b4 into main Nov 4, 2021
@aeschli aeschli deleted the aeschli/browseThemes branch November 4, 2021 11:58
@github-actions github-actions bot locked and limited conversation to collaborators Dec 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
themes Color theme issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants