Workspace theme #3093
Merged
Workspace theme #3093
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alschmiedt
approved these changes
Sep 26, 2019
core/theme_manager.js
Outdated
| }; | ||
|
|
||
| /** | ||
| * Set the workspace theme, and refresh the workspace. |
There was a problem hiding this comment.
'and refresh all subscribed workspaces and components' ?
core/workspace.js
Outdated
| /** | ||
| * Updates all the blocks with new style. | ||
| * @param {!Array.<!Blockly.Block>} blocks List of blocks to update the style | ||
| * on. |
core/toolbox.js
Outdated
| @@ -175,7 +178,8 @@ Blockly.Toolbox.prototype.init = function() { | |||
| oneBasedIndex: workspace.options.oneBasedIndex, | |||
| horizontalLayout: workspace.horizontalLayout, | |||
| toolboxPosition: workspace.options.toolboxPosition, | |||
| renderer: workspace.options.renderer | |||
| renderer: workspace.options.renderer, | |||
| theme: workspace.options.theme | |||
There was a problem hiding this comment.
Do we need this here? I think the workspace is just going to end up using the parent's themeManager
There was a problem hiding this comment.
Yep, I got rid of all the others, this one snuck through.
alschmiedt
reviewed
Sep 26, 2019
core/block.js
Outdated
| @@ -990,10 +990,10 @@ Blockly.Block.prototype.setColour = function(colour) { | |||
| * @throws {Error} if the block style does not exist. | |||
| */ | |||
| Blockly.Block.prototype.setStyle = function(blockStyleName) { | |||
| var theme = Blockly.getTheme(); | |||
| var theme = this.workspace.getTheme(); | |||
| if (!theme) { | |||
There was a problem hiding this comment.
Do we need this check anymore? Since theme should never be able to be null?
alschmiedt
reviewed
Sep 26, 2019
core/workspace.js
Outdated
| */ | ||
| this.themeManager_ = this.options.parentWorkspace ? | ||
| this.options.parentWorkspace.getThemeManager() : | ||
| new Blockly.ThemeManager(this.options.theme); |
There was a problem hiding this comment.
Should we set a default theme here if there are none on the options? There was a bug that the code we deleted fixed for headless workspaces. Issue #2605
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The basics
The details
Resolves
#2733
Proposed Changes
Move the theme so it's attached to the parent workspace (and no longer a global). This allows us to have multiple instances of Blockly with different themes.
Created a new object
Blockly.ThemeManagerthat stores the current workspace theme and is in charge of registering subscribed workspaces, and registering subscribed UI components.Subscribed workspaces are refreshed when the theme changes.
Throughout our codebase we can also register Blockly UI components with the theme, letting the theme manager know what component name to use, and what style property to set to. Whenever the theme changes, the UI component is updated with the new value for the component name it's interested in.
This happens at runtime, so developers are able to switch themes, and have all the styles reflected.
Added Dark theme for testing.
Reason for Changes
No way to configure UI component styles.
Test Coverage
Testing with Dark Theme.
Tested playground and multi-playground.
All tests pass.
Tested on:
Documentation
Need to add documentation for how to configure themes.
Additional Information