Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Fixed status bar so it can add indicators to itself again #6304

Merged
merged 5 commits into from
Dec 27, 2013
Merged

Fixed status bar so it can add indicators to itself again #6304

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c47783e
addIndicator now adds to DOM, add special case for CodeInspection module
lkcampbell Dec 22, 2013
9ffd031
Small code cleanup
lkcampbell Dec 22, 2013
462bea2
Make sure _beforeID element exists before inserting indicator before it
lkcampbell Dec 23, 2013
77a0364
Add public parameter insertBefore to StatusBar.addIndicator()
lkcampbell Dec 24, 2013
7c4197f
Update 'busy' indicator comments
lkcampbell Dec 27, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/language/CodeInspection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,7 @@ define(function (require, exports, module) {

// Status bar indicator - icon & tooltip updated by run()
var statusIconHtml = Mustache.render("<div id=\"status-inspection\">&nbsp;</div>", Strings);
$(statusIconHtml).insertBefore("#status-language");
StatusBar.addIndicator(INDICATOR_ID, $("#status-inspection"));
StatusBar.addIndicator(INDICATOR_ID, $(statusIconHtml), true, "", "", "status-language");

$("#status-inspection").click(function () {
// Clicking indicator toggles error panel, if any errors in current file
Expand Down
12 changes: 10 additions & 2 deletions src/widgets/StatusBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,13 +98,14 @@ define(function (require, exports, module) {
* @param {boolean=} visible Shows or hides the indicator over the statusbar.
* @param {string=} style Sets the attribute "class" of the indicator.
* @param {string=} tooltip Sets the attribute "title" of the indicator.
* @param {string=} _beforeID For internal use by CodeInspection module.
*/
function addIndicator(id, indicator, visible, style, tooltip) {
function addIndicator(id, indicator, visible, style, tooltip, _beforeID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the preceding underscore should be removed. I don't see any reason why extensions shouldn't be allowed to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, it will be an API addition then. I have to make sure I understand exactly how it is to work so I name and document it correctly. When it was internal use, didn't have to worry about it.

First off, when I say BEFORE an indicator, I mean "to the left of" an indicator in the user interface. When I say AFTER an indicator, I mean "to the right of" an indicator in the user interface. This needs to be clarified because the DOM model of the indicator elements runs top-to-bottom, while the view of the indicators runs right-to-left. Makes it easy to confuse jQuery concepts of BEFORE and AFTER, with UI concepts of BEFORE and AFTER.

It seems like relative positioning should really have the ability to specific FIRST, LAST, and BEFORE, or AFTER, an existing element. So, one question is, is there any place we do not want to have extensions inserting indicators?

For example, putting an indicator FIRST would put it BEFORE the "busy" indicator. That would probably look weird.

Another example, putting an indicator LAST would put it AFTER the space/tab indicator. It seems like this messes with the user experience a bit.

But do we want to constrain relative positioning? Or do we allow it and let issue reports from users deal with the odd cases that crop up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon further reflection (i.e. a good night's sleep) we can probably pull this off with just beforeID. Except I am going to call it insertBefore because it is similar nomenclature to the jQuery method. In fact, I am going to paraphrase the jQuery documentation description as well because the purpose of this parameter is surprisingly difficult to describe without tying the sentence into knots:

An id of an existing status bar indicator. The new indicator will be inserted before (i.e. to the left of) the indicator specified by this parameter.

Ugh. Not great, but close enough.

The main side effect of only using insertBefore is that it is impossible to insert any indicator AFTER the space/tab indicator. I think this is a good thing though. It keeps the space/tab indicator in the lower right corner at all times.

We do need to decide, however, if we are going to prevent an insert BEFORE the "busy" indicator. If so, I need to update the parameter documentation a little and tweak the code. If not, we should be good to go with this next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, we don't need to worry about inserting before the "busy" indicator because that indicator doesn't have an id. I suppose they could, theoretical, set the id to "status-bar .spinner", but the code discourages this behavior. So, we should be set with this next commit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with keeping this simple.

if (!_init) {
console.error("StatusBar API invoked before status bar created");
return;
}

indicator = indicator || document.createElement("div");
tooltip = tooltip || "";
style = style || "";
Expand All @@ -121,6 +122,13 @@ define(function (require, exports, module) {
$indicator.hide();
}

if (_beforeID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If _beforeID is specified, but doesn't exist, then indicator won't get inserted. So, what do you think about checking for it like this:

    if (_beforeID && $("#" + _beforeID).length > 0) {

$indicator.insertBefore("#" + _beforeID);
} else {
// The "busy" spinner should always be leftmost
var $busyIndicator = $("#status-bar .spinner");
$indicator.insertBefore($busyIndicator);
}
}

/**
Expand Down