Fixed status bar so it can add indicators to itself again #6304
Conversation
|
Just a couple of notes on this. @njx indicated that he only wanted relative positioning for core features, so I named the new parameter for addIndicator with an underscore name I didn't add the new indicator to the very leftmost position, because I thought it would look weird if the "busy" indicator appeared in the middle of a bunch of indicators. So, I kept the "busy" indicator as the leftmost indicator always. If you don't like this, let me know and I can change it. |
| @@ -121,6 +122,13 @@ define(function (require, exports, module) { | |||
| $indicator.hide(); | |||
| } | |||
|
|
|||
| if (_beforeID) { | |||
There was a problem hiding this comment.
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) {
|
@redmunds, code review change committed. |
| */ | ||
| function addIndicator(id, indicator, visible, style, tooltip) { | ||
| function addIndicator(id, indicator, visible, style, tooltip, _beforeID) { |
There was a problem hiding this comment.
I think the preceding underscore should be removed. I don't see any reason why extensions shouldn't be allowed to use it.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree with keeping this simple.
|
@redmunds, code review changes committed. |
| if (insertBefore && $("#" + insertBefore).length > 0) { | ||
| $indicator.insertAfter("#" + insertBefore); | ||
| } else { | ||
| // No positioning is provided, put after "busy" indicator |
There was a problem hiding this comment.
I'd like a little more detail regarding positioning of busy indicator. Maybe:
// No positioning is provided, put on left end of indicators, but
// to right of "busy" indicator (which is usually hidden).
|
Done with review. Code looks good. Just one nit about comments. |
|
@redmunds, code review changes committed. |
ghost
assigned
|
Looks good. Merging. |
Fixed status bar so it can add indicators to itself again
This fix addresses issue #5682. StatusBar.addIndicator() now adds the indicators to the DOM correctly.