bartbutenaers / node-red-contrib-blockly

A Node Red node for visual programming a function using Blockly
Apache License 2.0
88 stars 22 forks source link

Themes selection #106

Closed bartbutenaers closed 10 months ago

bartbutenaers commented 10 months ago

When people use e.g. a dark Node-RED theme, the Blockly editor colors don't really match with that theme:

image

Recently a series of Blockly themes plugins have been released.

In the blockly-themes branch of this repository, I have already implemented some changes to support those themes in this node:

  1. The package.json file downloads all theme plugins as dependencies:

    "@blockly/theme-dark": "^6.0.1",
    "@blockly/theme-highcontrast": "^5.0.1",
    "@blockly/theme-tritanopia": "^5.0.1",
    "@blockly/theme-deuteranopia": "^5.0.1",
    "@blockly/theme-modern": "^5.0.1",
  2. The config node has a dropdown to select one of the available themes:

    image

    The default theme is "classic".

  3. The blockly.config file has been updated to:

    • Get the selected theme name from the config node.
    • Load the corresponding blockly plugin as ES6 module from the backend
    • Pass the blockly theme instance as a parameter to the Blockly.inject function

But unfortunately the workspace view layout is messed up then. If anybody wants to help solving it, that would be appreciated very much!!!

ralphwetzel commented 10 months ago

Hi @bartbutenaers!

There's good news for you and those using your great node. ๐Ÿ˜„ Theme support is possible! Watch this:

BlocklyThemes

I found two reasons why the workspace is messed up with your current setup:

The second bullet is for you to figure out. You'll find those portions commented. I've solved the first for you - with an ugly, but successful hack!

Expect a PR soon!

bartbutenaers commented 10 months ago

Really cool! You are my hero for today ;-)

bartbutenaers commented 10 months ago

I will keep this issue open until the second bullet is solved.

bartbutenaers commented 10 months ago

@ralphwetzel: Could you do me one more favor. Which steps do I need to follow to reproduce the error? I have seen recently some issues on the Blockly forum about disposing some plugins. So perhaps something is wrong about that yet.

ralphwetzel commented 10 months ago

It's enough to remove the comment tags from lines 221 - 229:

https://github.com/bartbutenaers/node-red-contrib-blockly/blob/c622d5b0f4c9c421e98b156b4beca524239ec5c9/blockly.html#L216-L229

Re-enabling searchElement then triggers the following error on my system:

image
ralphwetzel commented 10 months ago

I just did some further tests. Looks like this issue is related to the fact that we load the themes immediately before the definition of searchElement. I'm not sure why - but moving

https://github.com/bartbutenaers/node-red-contrib-blockly/blob/c622d5b0f4c9c421e98b156b4beca524239ec5c9/blockly.html#L214

after searchElement definition doesn't trigger the error. I'll send you another PR.

cymplecy commented 10 months ago

Minor issue - field is blank on exisiting node image

bartbutenaers commented 10 months ago

@cymplecy: i will add tonight a statement to put a default value "classic" to this field when it is undefined. Similar to the offset ยด0ยด like last week.

ralphwetzel commented 10 months ago
image

I have the impression that implementing the theme loader with an async/await pattern caused this issue. As I'm not convinced that the "fix" proposed in my last PR is fully reliable (it might be for this case, but none knows what happens in the future!), I've re-written the loader to run as a sync process. This might look a bit "old fashioned", but follows the way other resources are loaded, too.

bartbutenaers commented 10 months ago

Minor issue - field is blank on exisiting node

@cymplecy, that should be fixed now in the github version. If you have some time to test it and also test the theme mechanism, that would be fine!

I've re-written the loader to run as a sync process

Really really appreciated! I would have to put a lot of other stuff on pauze, if you wouldn't have solved all of this...

BTW you have also updated the position of the expand button from 'in' the tabsheet to 'above' the tabsheet:

image

Is that the intention, or does it perhaps looks different on your system? At the time being I had added it on top of the tabsheet, to save space to be able to show the Blockly workspace as large as possible. Moreover that way it was more clear that only the first tabsheet could be expanded...

ralphwetzel commented 10 months ago

๐Ÿ˜† My intention was to move the expand button from 'under' to 'on' the tab. And it sits there perfectly - on my system (NR 3.1, MacOS):

Chrome:

image

Safari:

image

Firefox:

image

EDIT: ... and on a MacOS system running NR 4.0-dev I get what you get: ๐ŸŽ‰

image

I'll try to figure out what's happening...

bartbutenaers commented 10 months ago

Ah it was originally under your tabsheet. I wasn't aware of that... Now I understand the reason behind your change ๐Ÿ˜€

cymplecy commented 10 months ago

@bartbutenaers Theme is defaulting to classic if not previously set in the config :)

bartbutenaers commented 10 months ago

@ralphwetzel: I can confirm that the expand button is now also correctly positioned inside the tabsheet, on my Windows 10 portable. Kudos!!!

@cymplecy: Am I correct that everything is solved from the above discussion. If so, you may close this issue. Thanks!

bartbutenaers commented 10 months ago

@cymplecy; if you can also check my last question, so that we can finalize our release... Thanks!

cymplecy commented 10 months ago

Ignore missing theme selector report if you see it-PEBKAC here :)