-
-
Notifications
You must be signed in to change notification settings - Fork 489
Live change of theme from Preferences dropdown #1296
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a try on Windows and it works perfectly for me. Very cool enhancement to the IDE.
Thanks Francesco!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It worked for me. I have verified it from the IDE2 started from the sources. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kittaakos is the method .setCurrentTheme also called by Theia code after we click apply on the settings dialog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
considering the proposed changes maybe we should consequently mod this also:
<select
className="theia-select"
value={
ThemeService.get()
.getThemes()
.find(({ id }) => id === this.state.themeId)?.label ||
nls.localize('arduino/common/unknown', 'Unknown')
}
onChange={this.themeDidChange}
>
could be:
<select
className="theia-select"
value={
ThemeService.get().getCurrentTheme().label
}
onChange={this.themeDidChange}
>
I don't see a reason to now reference the state prop themeId in the JSX (though maybe I'm missing something).
p.s. I don't know if || nls.localize('arduino/common/unknown', 'Unknown') is really required, here I've assumed not.
I agree @davegarthsimpson. I applied the change here, I tested it and it works fine.
Uh oh!
There was an error while loading. Please reload this page.
Motivation
The theme must be updated immediately if the user selects it from the Preferences drop-down menu. In this way it's possible to preview the appearance of the various themes available.
By pressing the Cancel button the initial theme must be restored.
Change description
Restore the previous theme when the dialog is closed by pressing the Cancel button.
Other information
This is an ad hoc solution for themes, in the long run this behavior should be generalized because other elements of the Preferences, such as the Editor font size or the Interface scale, may also require a live update to give immediate feedback to the user. In this case maybe a big rework of settings is needed.
Closes #1048.
Reviewer checklist