Fix: Migrating every theme to TypeScript.#2474
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hi @tgbhy. Thanks for this useful PR! The changes look good but there are some additional steps needed before we can accept the PR:
|
|
Working on it ! |
|
This is fine now ? |
|
Wait, it's already the case ? |
|
Bump here |
|
Hi @tgbhy we can't merge this because renaming the properties will break them. I'm not sure how you tested the change, but the way themes work are that they correlate a category name to a set of styles. The category names like We can accept this PR after you address Christopher's comment here https://github.com/google/blockly-samples/pull/2474/files#r1929130761 Please let us know if you have further questions. Thanks for your work on this and for following up! |
|
oh okay, I don't know why it was working perfectly for me... |
|
You will need to fix this for all of the |
|
So wait, I need to do this only for Because I really don't understand why you ask me that because it was working fine for me but whatever, just confirm me that and I'll do the changes. |
|
You can check how a theme is supposed to look on the published plugin showcase page. Here's what the highcontrast theme is supposed to look like, for example: https://google.github.io/blockly-samples/plugins/theme-highcontrast/test/index.html You can test your change by running All of the blocks are black and the toolbox is missing the little color stripe for the category. This means your theme is broken. You can read more about how themes work here. The properties in the theme like This PR shouldn't change any object keys as part of the change. You should revert all of those kinds of changes. If the key was previously |
|
Okay thank you, I will do all the edits right now. |
|
And done ! |
|
Give me a sec I have an idea |
|
And the work is done ! |
|
Any news here ? |
cpcallen
left a comment
There was a problem hiding this comment.
Thanks for the changes; they look great—and sorry for the delayed review: we were busy with the Blockly v12 release.
|
No problem. Will it be merged now, or does it need more modifications? |

The basics
The details
Resolves
Fixes #2158
Proposed Changes
Migrate every theme sample to TypeScript.
Reason for Changes
It would be usable in TypeScript like explained in the fixed issue.
Test Coverage
Manually tested with a Windows 10 PC and Brave Browser.
I tested on using a default installation of Blockly and recreating the schema of each readme-media/Theme.png