Add/expose isVisibile, show, and hide methods for SidebarView object #3297
Conversation
ghost
assigned 
|
@lkcampbell This looks pretty good.
UPDATE: Nevermind. That won't work since it's already a jQuery object. |
|
Looks good. Merging. |
Add/expose isVisibile, show, and hide methods for SidebarView object
| * Returns the visibility state of the sidebar. | ||
| * @return {boolean} true if element is visible, false if it is not visible | ||
| */ | ||
| function visible() { |
There was a problem hiding this comment.
I know this was merged already, but naming this function isVisible() would have been better, since it checks for the visible state and returns a boolean.
The same would apply to the new method in Resizer.js.
Maybe it could be changed as code cleanup before the Sprint ends.
There was a problem hiding this comment.
I don't have a strong opinion either way. They both sound good to me.
If you guys want me to change the name I will.
There was a problem hiding this comment.
Good catch, @TomMalbran. @lkcampbell Yes, please change it to isVisible().
|
@redmunds and @TomMalbran, new pull request with cleanup submitted. |
Cleanup for pull request #3297
For Issue #1187
Added public API functions for SidebarView object: show(), hide(), and visible().
Changed public API function: toggleSidebar() to toggle().
Did a find in files for current Brackets codebase and all currently listed Brackets extensions. toggleSidebar() is not used anywhere except for locally in SidebarView.js.
Ran unit tests without any problems. I'm not sure that SidebarView has any unit tests right now. If you want me to research and possibly create unit tests I can. Otherwise, this fix is complete and ready for review.