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

[Settings - Themes]: Implementing dropdown in settings to change themes #229

Merged
merged 8 commits into from
Oct 30, 2020

Conversation

mliao95
Copy link

@mliao95 mliao95 commented Oct 26, 2020

This PR adds a settings dropdown to change the themes of the DevTools extension.
image

This PR also broadens the use of the settingsProvider (formerly tabSettingsProvider) to include the theme settings.

Closes #207

@mliao95 mliao95 changed the title Implementing dropdown in settings to change themes [Settings - Themes]: Implementing dropdown in settings to change themes Oct 27, 2020
@mliao95 mliao95 force-pushed the user/milia/theme-settings branch from 8392e6d to d57723e Compare October 29, 2020 18:42
.vscode/settings.json Outdated Show resolved Hide resolved
vidorteg
vidorteg previously approved these changes Oct 30, 2020
Copy link
Contributor

@vidorteg vidorteg left a comment

Choose a reason for hiding this comment

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

LGTM % comments

console.log('Theme =' + theme);
resolve(theme);
});
}.bind(this));
Copy link
Author

Choose a reason for hiding this comment

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

@vidorteg - I checked, and we do need this bind call to access the getThemes function

@mliao95
Copy link
Author

mliao95 commented Oct 30, 2020

In the most recent patch, I did some things to clean up the code:

  • Differentiated between themeString and theme in both the extension code and injected code
    • themeString will be the user-visible string (e.g. "System preference") and the theme will contain the id string (e.g. "systemPreferred")
  • Moved the switch logic into toolsHost.ts
    • seems to make more sense to send over the needed theme string rather than having logic to parse it injected into the source code.

@mliao95 mliao95 requested a review from vidorteg October 30, 2020 20:18
@mliao95 mliao95 force-pushed the user/milia/theme-settings branch from afa9438 to 7a5250b Compare October 30, 2020 20:23
@mliao95 mliao95 merged commit 973927c into master Oct 30, 2020
@mliao95 mliao95 linked an issue Nov 2, 2020 that may be closed by this pull request
@vidorteg vidorteg deleted the user/milia/theme-settings branch January 30, 2024 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants