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

[CLOSED] StatusBar.addIndicator() doesn't actually add the indicator #5210

Open
core-ai-bot opened this issue Aug 30, 2021 · 14 comments
Open

Comments

@core-ai-bot
Copy link
Member

Issue by njx
Friday Oct 25, 2013 at 01:01 GMT
Originally opened as adobe/brackets#5682


Originally, when StatusBar.addIndicator() was implemented, it would actually add the indicator into the status bar. However, in d7bce2cee3241cf3152e15cbfbcf66476dac23cb, that line was removed, which is a breaking API change. It's not clear why it was removed.

(Edited 12/14/2013 by@lkcampbell to add repro steps)

OS Version: Mac OSX Mavericks

Brackets Version: Sprint 34

Repro Steps:
Open Brackets source code, then open Dev Tools to Console and paste the following code snippet into the Console:

var StatusBar = require("widgets/StatusBar"),
    $foo = $("<div>FOO</div>");
StatusBar.addIndicator("foo", $foo, true, "", "foo tooltip");

Look at the right side of the editor status bar.

In the Dev Tools, Elements tab, look at the contents of status-indicators div tag.

Observed Result:
The FOO div does not show up in the status bar and is not in the status-indicators div tag

Expected Result:
The FOO div shows up in the status bar and is in the status-indicators div tag

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Oct 25, 2013 at 01:01 GMT


@larz0 - do you know why you removed this?

@core-ai-bot
Copy link
Member Author

Comment by larz0
Friday Oct 25, 2013 at 02:09 GMT


@njx I think it was because it was inserting something else that's not supposed to be there. I can't remember what though. Could it be related to JSLint indicator?

@core-ai-bot
Copy link
Member Author

Comment by larz0
Friday Oct 25, 2013 at 03:18 GMT


@njx I put it back in on this branch https://github.com/adobe/brackets/tree/larz/issue-5682 and there are no visible changes.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Wednesday Nov 06, 2013 at 14:33 GMT


Assigning this issue to myself to fix, I need resolved so I can address issue #5078.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Sunday Nov 10, 2013 at 03:27 GMT


@njx, question for you. What repro steps are we using to test it and make sure I fixed it? This is the second issue I have run into where I need to create test steps for a Brackets module and I have no idea how to do it.

Do you guys have some way to conduct a headless test on some specific piece of the Brackets API?

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Dec 14, 2013 at 16:24 GMT


Edited the description to include repro steps.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Saturday Dec 14, 2013 at 17:10 GMT


@njx and@larz0 , this problem is caused by what@njx mentioned above: the $indicators.prepend($indicator); line was removed from StatusBar.addIndicator().

But I also discovered the reason why@larz0 pulled this code in the first place.

This is what the status bar indicators look like normally:

screen shot 2013-12-14 at 8 45 32 am

This is what the status bar indicators look like after inserting the prepend line again:

screen shot 2013-12-14 at 8 42 47 am

The problem is addIndicator() doesn't have a way to indicate relative position. The default behavior is to push indicators onto the right side. It is a little counterintuitive because children of the status-indicators div are drawn right to left. That is to say, the first child is drawn on the rightmost side and the last child in drawn on the leftmost side.

We must have some special code that sets the JSLint indicator position manually right now. Reactivating the default behavior of addIndicator() overrides it.

My question is, how do you want me to fix this problem? What is the expected behavior? If the JSLint indicator is to remain in the middle, I may have to add relative positioning to the addIndicator() method interface. Any other ideas?

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Friday Dec 20, 2013 at 00:04 GMT


@njx or@redmunds, Randy was asking about the status of the "Reload Without User Extensions" feature today. This issue is one of the blocking issues for the feature. I am looking for feedback from you regarding the best way to address this problem. Please see my questions above.

@core-ai-bot
Copy link
Member Author

Comment by njx
Friday Dec 20, 2013 at 00:12 GMT


Sorry, forgot to follow up. A few thoughts:

  • I think it makes sense to add indicators from right-to-left, since these indicators are right-justified. So addIndicator() should add the new indicator to the left side of the group.
  • I'm okay adding a relative position to addIndicator() (e.g. by ID), though I think we should really only use this for our core features. Third-party extensions shouldn't care too much about where they get added.

@core-ai-bot
Copy link
Member Author

Comment by lkcampbell
Sunday Dec 22, 2013 at 15:35 GMT


@njx and@redmunds, fix committed.

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Friday Dec 27, 2013 at 17:34 GMT


FBNC back to@njx

@core-ai-bot
Copy link
Member Author

Comment by njx
Thursday Jan 09, 2014 at 23:30 GMT


Haven't tried it myself but the code looks sensible. Closing.

@core-ai-bot
Copy link
Member Author

Comment by peterflynn
Friday Jan 10, 2014 at 01:54 GMT


@lkcampbell@redmunds Do you think this is clean enough to be an official API for use by extensions now? If so, would one of you mind adding it to the release notes as a new API?

@core-ai-bot
Copy link
Member Author

Comment by redmunds
Saturday Jan 11, 2014 at 18:57 GMT


@peterflynn Done.

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

No branches or pull requests

1 participant