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

add hideprev and hideafter to uiLayoutContainer #158

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

add hideprev and hideafter to uiLayoutContainer #158

wants to merge 6 commits into from

Conversation

zsp1987
Copy link

@zsp1987 zsp1987 commented Nov 9, 2015

add hideprev and hideafter attributes to uiLayoutContainer to control hide "prev" or "after" icon of the following spliterbar.

add `hideprev` and `hideafter` attributes to uiLayoutContainer to control hide "prev" or "after" icon of the following spliterbar.
@petrsimon
Copy link
Contributor

This is an interesting idea, but I see two problems.

  1. the scope you are working with is actually inherited from uiLayout, so you'll be changing all splitbars, right?
  2. you are initializing scope.hideprev and scope.hideafer using !== undefined, which will lead to unintuitive behaviour, because falsy values will set it to true.

@zsp1987
Copy link
Author

zsp1987 commented Nov 10, 2015

  • For each uiLayoutContainer, it generate a separate splitterbar with its parent uiLayoutContainer scope. So hideprev and hideafter should have the same scope with its parent uiLayoutContainer but not share a common scope with other.
var splitbar = angular.element('<div ui-splitbar><a ng-show="!hideprev"><span class="ui-splitbar-icon"></span></a>'
                                            + '<a ng-show="!hideafter"><span class="ui-splitbar-icon"></span></a></div>');

element.after(splitbar);
$compile(splitbar)(scope);
  • I am not sure, because at beginning I thought ng-show="hideprev" with a false value is also weird.I can change it or any better suggestion?

@petrsimon
Copy link
Contributor

Ah sorry, you are right. My bad.

I think I see a glitch, though. I think that hideprev won't always work, because the splitbar is compiled with the scope of the previous container. That means that if you have 3 containers, hideprev in the second hides the prevButton of the following splitbar and hideprev in the third container does nothing. That seems confusing to me.
Your proposal basically allows to create something like accordion with pinned panes and I think that makes sense. Obviously, you find it useful!
You are right that the attribute names are little confusing. You are setting up a container and suddenly you are asked to decide if you want to hide something "prev" or "after"... Since the splitbar is using the previous container's scope, there could be a directive or attribute for setting up the splitbar, like splitbar="{hidePrevButton: false, hideAfterButton: true}" or something like that. That would make it clearer. And then you could ng-hide="hidePrevButton". Or maybe shorter splitbar="{prevButton: false, nextButton: true"} with ng-show="afterButton".

Actually, why is if(0 < index && !ctrl.hasSplitbarBefore(scope.container)) even there? When does this happen? After container is removed? Have to look at it later with a clear head.

A side note: I was actually thinking about something like your proposal myself. In #154 I've introduced a notion of a "central" container, which doesn't implement the hiding, but it's motivated by what other layout libraries, like jquery ui-layout, have. A central container which always takes up the remaining space (and can never be closed - this I have not implemented yet, I simply hide the splitbar). That's the common theme we see around (the editor is central and around it a file tree, symbol tree, toolbar, statusbar, etc. all of which can be closed, except for the editor). I'm planning to extend the "central" container by disabling the inward toggles around it.

@SomeKittens
Copy link
Contributor

Sorry I haven't been keeping up with things lately, should be back on track now. @petrsimon thanks for doing some review!

@zsp1987 This looks useful and I'd love to bring it in if we can get over the bumps mentioned by @petrsimon .

…button.

```
<div ui-layout-container splitbar="{prevButton: false}"></div>
```
@zsp1987
Copy link
Author

zsp1987 commented Nov 14, 2015

@petrsimon Thank you for the advices. Add splitbar attribute with options to uiLayoutContainer is much clear. Have implemented this.

if(0 < index && !ctrl.hasSplitbarBefore(scope.container)) I guess now. Each uiSplitbar is immediate follows a uiLayoutContianer. So the splitbar indeed does not function well for the last one.

@SomeKittens
Copy link
Contributor

@zsp1987 I like how this is coming together. Can you add some documentation about your new feature to the readme?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants