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

Adding new features to version 1.1.0 #58

Merged
merged 16 commits into from
May 27, 2021

Conversation

jsccjj
Copy link
Contributor

@jsccjj jsccjj commented Apr 24, 2021

Hi Bart,

I spent some free time looking into your wonderful package. I modified some codes and added new features based on version 1.1.0.

Major modifications (in my opinion):

  1. Upgraded Blockly to version "5.20210325.1"
  2. Added an Ace XML editor. Now load-from/save-to library is enabled. Saved files are XML that can properly re-create Blockly workspace
  3. Created an expand button for the Blockly workspace using Red.trays api

Minor modifications:

  1. When loading XML toolbox files, parse the categories in a forward fashion instead of backward so that nested categories can be parsed properly
  2. used Blockly.Xml.domToPrettyText instead of Blockly.Xml.domToText

I hope this may add value to you package.

Thanks,
Jeff

@bartbutenaers
Copy link
Owner

Morning Jeff,

A fullscreen-mode was for me the most important requirement, but I was completely stuck with. Even with the help from the Blockly team (see issue 1 and issue 2). As a result the devolpment of this node was completely halted, and I had to disappoint my partner
Simon (@cymplecy) several times. I think he already gave up all hope for a new major version of this node...

But seems you have managed to implement this feature. Awesome!!! Thank you so much!! Moreover this is the first large pull request that I have ever received on any of my nodes...

I am not developing at the moment, since I'm recovering from an operation. But if everything goes well, I hope to start working on a new release of this node in around 6 weeks from now.

I have not reviewed your code in detail, but could you:

  • Add yourself to the contributor section. I think you have earned that title ...
  • Seems you have added some extra blocks. Can you explain those, so Simon can give his 'functional' feedback.
  • You expand only the Blockly editor tabsheet. I think it is usefull that the full-screen mode contains the 3 tabsheets. Suppose I want to learn programming Javascript. I start with this node, and every time that I have dragged a block in the editor, I want to switch to the "Javascript" tabsheet over and over again. But this now means I have to leave and enter your fullscreen mode every time, which is not really user friendly. Is it possible to show the tabsheet control with 3 tabsheets in full-screen??
  • You have upgraded the Blockly version. Do you have seen any other new features that we should support?

I will try to review your changes in detail as soon as possible. Will get back to you with my feedback...

Have a nice sunday,
Bart

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 26, 2021

Hi Bart,

Thank you for your recognition! I am glad that I could contribute. Also, my best wishes to you for your recovery from the operation.

Please see my responses below:

- Add yourself to the contributor section. I think you have earned that title ...

Jeff: My pleasure!

- Seems you have added some extra blocks. Can you explain those, so Simon can give his 'functional' feedback.

Jeff: I removed most of the added blocks since they are only useful for my building automation projects.
I only kept "Javascript Expression" block

image
This is a, I think, a complementary block that is useful while users want to use methods such as isNaN(), isFinite(), and etc with Blockly core blocks.

- You expand only the Blockly editor tabsheet. I think it is usefull that the full-screen mode contains the 3 tabsheets. Suppose I want to learn programming Javascript. I start with this node, and every time that I have dragged a block in the editor, I want to switch to the "Javascript" tabsheet over and over again. But this now means I have to leave and enter your fullscreen mode every time, which is not really user friendly. Is it possible to show the tabsheet control with 3 tabsheets in full-screen??

Jeff: I created the expand view by using Red.tray() directly. Creating tabs in the expanded view reduce the editor area and make the code much longer since we have to redefine tabs as well as html part of it. So, I just created an alternative by using buttons:

image

By clicking "See Generated Javascript" button, users can get to the readonly Javascript editor:

image

Users can get back to the Blockly workspace by clicking "Back to Blockly Editor".

I have committed this change to my fork. I will create another pull request if you think it's appropriate.

- You have upgraded the Blockly version. Do you have seen any other new features that we should support?

Jeff: I have not investigated into it yet. Is there any features you are expecting?

@bartbutenaers
Copy link
Owner

So, I just created an alternative by using buttons

That looks very user-friendly to me. Nice development work!!!

I will create another pull request if you think it's appropriate.

No please do it all in this PR. That makes it easier for me to review it afterwards.

Is there any features you are expecting?

I was just wondering if you had upgraded a new version of Blockly because you required new features. But now I assume you just wanted to get my node in sync with Blockly.

Last year I had already implemented some changes (in a local version on my pc):

  • Extra nodes since the API of the Function-node sandbox has been extended with new features.
  • Removed the Blockly libs from this repo, and implemented a Blockly NPM dependency (so new minor Blockly versions are automatically installed).

I will commit those changes to Github when I'm back into business...

@cymplecy
Copy link
Collaborator

Hi Jeff
1st thoughts
image

Is different order - could it be changed to the original order
image

Also, the resizing buttons could do with being fully visible without need to scroll
image

@cymplecy
Copy link
Collaborator

On a separate issue the extra JavaScript function block comes up from time to time but it's never made it into the project as it's not really needed.

If you think that you have a use case that requires it - let me know what it is and I'll show you that it isn't needed :)

Simon

@cymplecy
Copy link
Collaborator

And your JS blocks in your example aren't needed :)

image

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 26, 2021

So, I just created an alternative by using buttons

That looks very user-friendly to me. Nice development work!!!

Thank you!

I will create another pull request if you think it's appropriate.

No please do it all in this PR. That makes it easier for me to review it afterwards.

Got it. I just double checked that the pull request is reflecting my latest commits.

Is there any features you are expecting?

I was just wondering if you had upgraded a new version of Blockly because you required new features. But now I assume you just wanted to get my node in sync with Blockly.

Last year I had already implemented some changes (in a local version on my pc):

  • Extra nodes since the API of the Function-node sandbox has been extended with new features.
  • Removed the Blockly libs from this repo, and implemented a Blockly NPM dependency (so new minor Blockly versions are automatically installed).

I will commit those changes to Github when I'm back into business...

I look forward to it! Let us know what else we can contribute.

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 26, 2021

Hi Jeff
1st thoughts
image

Is different order - could it be changed to the original order
image

Hi Simon, I changed the order per your comment but left the "Misc" at the bottom for the reason that the toolbox of "Misc" contains a "Logic" type of block. When joining the toolboxes this may lead all "Logic" blocks into "Misc" category. Of course this can be changed again if you think it's more proper. New look is here:
image

Also, the resizing buttons could do with being fully visible without need to scroll
image

This part is fixed by changing the CSS style. The min-height setting of the expanded blockly view was set to 700px. Now, it is 350px. This should fix the problem.
Please let me know whether this works at your end.

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 26, 2021

On a separate issue the extra JavaScript function block comes up from time to time but it's never made it into the project as it's not really needed.

If you think that you have a use case that requires it - let me know what it is and I'll show you that it isn't needed :)

Simon

The "Javascript Statement" block is powerful already.

The situation I have is like:

Considering to generate the following code by blockly

if ((msg['topic']) == 'disabled_op') {
disabled_op = Number(msg.payload);
if (isNaN(disabled_op)) {
disabled_op = 0;
}
}

We can simply use the Statement block like this:
image
and get this:
image

Below would get the same thing
image
By using "Javascript Expression", I would need additional blocks to achieve the same goal. However, I view this is the purpose of Blockly that is to visually learn coding. If I had know how to write all the JS statements, I would just use the default function node.

Also, in the situation below, the expression block may be useful:
image
image

My two pennies here...

Thanks,
Jeff

@cymplecy
Copy link
Collaborator

My view is that the use of Blockly in Node-RED isn't to teach/learn JavaScript.

It is an alternative to using text-based JS and JSONata, so as to continue the low-code philosophy of Node-RED itself.

The JavaScript block is intended to be a block of last resort.

So if we need to return a value using it, then this sort of thing does the job
image

When someone finds a need for the JavaScript block, then this usually generates a discussion on whether what they are seeking to achieve should be provided by a built-in Blockly function or whether it's too rare a case to do so.

@cymplecy
Copy link
Collaborator

"Please let me know whether this works at your end."
The layout changes look/work fine for me :)

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 27, 2021

Hi Simon,
I understand. I will remove the node.

@bartbutenaers
Copy link
Owner

Hi Jeff,

Simon is completely right that the purpose of this node is an alternative for the function node, for those that don't feel confident with Javascript. But on the other hand we understand your use case.

So we had a very short discussion about it, and we agreed that you can keep your Expression-node. Consider it as a thank-you gift from our side, for all the time you have spend to help us improving this node ...

Hopefully you still have the source code somewhere ;-)

@jsccjj
Copy link
Contributor Author

jsccjj commented Apr 29, 2021

So we had a very short discussion about it, and we agreed that you can keep your Expression-node. Consider it as a thank-you gift from our side, for all the time you have spend to help us improving this node ...

Thank you Bart and Simon. I appreciate you guys' approval! I will add the code back :)

BTW, have you guys checked the "Multiline text input fields" which seems added to blockly last Nov.
https://developers.google.com/blockly/guides/create-custom-blocks/fields/built-in-fields/multiline-text-input

image

It seems it can be used on Comment and Statement blocks but may be incompatible to current blocks...

@bartbutenaers
Copy link
Owner

@cymplecy,
Is it ok for you if @jsccjj adds the multiline text input block in our toolbox?
I don't have any objections...

@cymplecy
Copy link
Collaborator

Fine by me :)

@jsccjj
Copy link
Contributor Author

jsccjj commented May 3, 2021

Hi guys,

I added the new block. Please give it a try.

the default:
image
the generated JS:
image

In action:
image
The generated JS:
image

Please let me know how you want to improve the block once you have tried it.

Thanks, Jeff

@cymplecy
Copy link
Collaborator

cymplecy commented May 4, 2021

Hi Jeff
I'm having an issue when using normal editor (not expanded one)
If make edits in Editor mode and then switch to Generated JavaScript mode and switch back to Editor mode - the edits are lost.
Simon

@jsccjj
Copy link
Contributor Author

jsccjj commented May 4, 2021

Hi Simon,
That was a bug. I got it fixed 2 days ago. Please clone my fork (https://github.com/jsccjj/node-red-contrib-blockly). And let me know if it works for you.
Sorry for the bug.

Thanks, Jeff

@cymplecy
Copy link
Collaborator

cymplecy commented May 4, 2021

I thought I was on the latest but I hadn't cleared my cache :)

All OK now :)

Copy link
Owner

@bartbutenaers bartbutenaers left a comment

Choose a reason for hiding this comment

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

Hi Jeff (@jsccjj),

Sorry for the long delay!
Your code changes are very nice!
I have added some remarks. Once those are solved, I will merge the pull request.
Then I will add some other stuff, to make sure we have a complete release.
Thanks !!
Bart

README.md Outdated Show resolved Hide resolved
lib/basic/msg/js/en.js Show resolved Hide resolved
lib/basic/msg/js/en.js Show resolved Hide resolved
@jsccjj jsccjj requested a review from bartbutenaers May 27, 2021 17:27
Copy link
Contributor Author

@jsccjj jsccjj left a comment

Choose a reason for hiding this comment

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

Hi Jeff (@jsccjj),

Sorry for the long delay!
Your code changes are very nice!
I have added some remarks. Once those are solved, I will merge the pull request.
Then I will add some other stuff, to make sure we have a complete release.
Thanks !!
Bart

Hi Bart,

No problem. I hope all is well over there!
I replied your comments. If you have any questions, please don't hesitate to let me know.

Thanks,

Jeff

lib/basic/msg/js/en.js Show resolved Hide resolved
lib/basic/msg/js/en.js Show resolved Hide resolved
@bartbutenaers bartbutenaers merged commit e5dc591 into bartbutenaers:release-1.1.0 May 27, 2021
@bartbutenaers
Copy link
Owner

Jeff,

I have a series of new features for this Blockly node in an old local version of mine at home. Would like to have those features also in the 1.1.0 release, so we can remove both old branches and my local changes. Would like to have a clean Github repository again, with only a master branch. That allows us to create smaller releases in the future, to avoid that we end up again with years of inactivity ...

Some of my old changes might not work anymore after your pull-request, so I will have to review those. Which might take some time.

I will create a series of issues in this repository, so you can do a follow-up and get an idea of the progress...

Thanks!!
Bart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants