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

Show binding changes #263

Merged
merged 13 commits into from
Jun 11, 2018
4 changes: 3 additions & 1 deletion lib/web_console/templates/_inner_console_markup.html.erb
Original file line number Diff line number Diff line change
@@ -3,6 +3,8 @@
<div class='console-actions'>
<div class='close-button button' title='close'>x</div>
</div>
<div class='console-inner'></div>
<div class='console-inner'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may drop this diff change as well, e.g. leave it as before:

<div class='console-inner'></div>

<div class='console-notifications'></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really need the extra layout here? What's so special about the notifications, they look like a regular console line to me.

Copy link
Author

Choose a reason for hiding this comment

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

Initially I had the console-notifications set to be updated whenever the binding changes, but it will disappear out of the scrolling window when the commands exceeds the scrollable view.

I have changed it to append before the lastChild of the prompt-box so that it remains in view, but I forgot to remove the unused console-notifications div. It is now removed!

</div>
</div>
<input class='clipboard' type='text'>
24 changes: 22 additions & 2 deletions lib/web_console/templates/console.js.erb
Original file line number Diff line number Diff line change
@@ -864,11 +864,23 @@ REPLConsole.prototype.scrollToBottom = function() {
this.outer.scrollTop = this.outer.scrollHeight;
};

// Change the binding of the console
/**
* Change the binding of the console
* Also updates a div to show that it has been changed.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may change, maybe we won't win a lot by changing the comment here.

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense, I'll revert the change here

*/
REPLConsole.prototype.switchBindingTo = function(frameId, callback) {
var url = this.getSessionUrl('trace');
var params = "frame_id=" + encodeURIComponent(frameId);
postRequest(url, params, callback);
var notification = document.createElement('div');
var _this = this;

notification.className = "console-notification";
Copy link
Collaborator

Choose a reason for hiding this comment

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

var notification = document.createElement('div');
notification.className = "console-notification";

var _this = this;
postRequest(url, params, function() {

Can we keep the flow like this here? The operations are "grouped" this way.

Copy link
Author

Choose a reason for hiding this comment

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

sure!

postRequest(url, params, function() {
notification.innerHTML = "Binding has changed to: " + callback();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm thinking whether we call this Context has changed to: because we cannot expect the web console users to know about bindings or the terminology around how code evaluation works in Ruby.

Copy link
Author

Choose a reason for hiding this comment

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

I initially had it as Context as well but renamed it last minute. I guess I can change it back! I agree it sounds more "intuitive" for people using web-console.

var lastPrompt = findLastChild(_this.inner, "console-prompt-box");
_this.inner.insertBefore(notification, lastPrompt);
_this.scrollToBottom();
});
};

/**
@@ -946,6 +958,14 @@ function removeAllChildren(el) {
}
}

function findLastChild(el, className) {
for (var i = el.childNodes.length - 1; i > 0; -- i) {
if (hasClass(el.childNodes[i], className)) {
return el.childNodes[i];
}
}
}

function findChild(el, className) {
for (var i = 0; i < el.childNodes.length; ++ i) {
if (hasClass(el.childNodes[i], className)) {
8 changes: 3 additions & 5 deletions lib/web_console/templates/error_page.js.erb
Original file line number Diff line number Diff line change
@@ -11,19 +11,17 @@ for (var i = 0; i < traceFrames.length; i++) {

// Change the binding of the console.
changeBinding(frameId, function() {
if (selectedFrame) {
selectedFrame.className = selectedFrame.className.replace("selected", "");
}

target.className += " selected";
// Rails already handles toggling the select class
selectedFrame = target;
return target.innerHTML;
});

// Change the extracted source code
changeSourceExtract(frameId);
});
}

// Change the binding of the current session and prompt the user.
function changeBinding(frameId, callback) {
REPLConsole.currentSession.switchBindingTo(frameId, callback);
}
1 change: 1 addition & 0 deletions lib/web_console/templates/style.css.erb
Original file line number Diff line number Diff line change
@@ -27,6 +27,7 @@
.console .clipboard { height: 0px; padding: 0px; margin: 0px; width: 0px; margin-left: -1000px; }
.console .console-prompt-label { display: inline; color: #FFF; background: none repeat scroll 0% 0% #333; border: 0; padding: 0; }
.console .console-prompt-display { display: inline; color: #FFF; background: none repeat scroll 0% 0% #333; border: 0; padding: 0; }
.console .console-notification { color: #FFF; }
.console.full-screen { height: 100%; }
.console.full-screen .console-outer { padding-top: 3px; }
.console.full-screen .resizer { display: none; }