Skip to content

Added general support for modal windows for variable rename/creation#396

Closed
MarkusBordihn wants to merge 1 commit intoRaspberryPiFoundation:masterfrom
MarkusBordihn:hotfix-variables
Closed

Added general support for modal windows for variable rename/creation#396
MarkusBordihn wants to merge 1 commit intoRaspberryPiFoundation:masterfrom
MarkusBordihn:hotfix-variables

Conversation

@MarkusBordihn
Copy link
Contributor

@MarkusBordihn MarkusBordihn commented Jun 2, 2016

This change will allow to pass over modal functions for:

  • window.alert
  • window.confirm
  • window.prompt

The expected format of these are the standard definition + callback (if needed) + opt title e.g.
alert
function(message, opt_title) { ...}

confirm
function(message, callback, opt_title) { callback(result); }

prompt
function(message, default, callback, opt_title) { callback(result); }

Example config
Blockly.inject(node, { modal: { alert: function(...) {...}, confirm: function(...) {...}, prompt: function(...) {...} } });

All my test works fine, with and without the modal parameter.

@carlosperate
Copy link
Contributor

There seems to be more unwanted changes in this PR, are you sure you were working on up-to-date master?

@MarkusBordihn
Copy link
Contributor Author

Sorry forgot to reset my branch, should be fixed now.

core/options.js Outdated
hasCss = true;
}
var hasModal = options['modal'];
if (hasModal === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be a callback to a modal then? If that's the case we need to make sure the interface is clearly defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, checked the current code and it seems we need something for:

  • window.alert
  • window.prompt
  • window.confirm

I will prepare this PR to support multiple version and the changes could be added in other PR if needed.

@carlosperate
Copy link
Contributor

I've added a couple of comments, but I haven't tested it as it is quite late on this side of the pond, I'll see if I can find some time tomorrow.

…sed in #393.

Added inject option "modal" which is used instead of window.prompt.
@MarkusBordihn
Copy link
Contributor Author

Thanks for your comments, added some additional changes and was able to solve some edge cases as well.

The new "modal" could be used to pass over modal functions for:

  • window.alert
  • window.confirm
  • window.prompt

The expected format of these are the standard definition + callback (if needed) + opt title e.g.
alert
function(message, opt_title) { ...}

confirm
function(message, callback, opt_title) { callback(result); }

prompt
function(message, default, callback, opt_title) { callback(result); }

Currently the modal options is only implemented for variable renaming and creation, but could be expand to any other function as well.

If the option is not set, the systems defaults are used.

@MarkusBordihn
Copy link
Contributor Author

MarkusBordihn commented Jun 3, 2016

@NeilFraser
As soon this PR is merged I will start to integrate the simliar approach to other scripts / blocks.
Maybe I will create new core file like core/modal.js

@MarkusBordihn
Copy link
Contributor Author

@NeilFraser
Friendly ping.

@NeilFraser
Copy link
Contributor

FYI, @rachel-fenichel is working on a new model for variable management. It is designed to optionally support typed variables so that C/Java/Fortran can be generated properly. The existing system was a quick and dirty implementation that lasted far longer than could have been predicted. I think it best to make one change to Rachel's new system, than change the system twice in succession.

@NeilFraser NeilFraser closed this Jun 17, 2016
@carlosperate
Copy link
Contributor

Any general ETA for the new variable system?

@rachel-fenichel
Copy link
Collaborator

Eta: sometime this summer.

Discussion at #446

@carlosperate carlosperate mentioned this pull request Jun 21, 2016
9 tasks
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.

4 participants