feat: Programmatically toggle containers #154

petrsimon · 2015-11-02T22:02:29Z

Enables:

programmatically un/collapse containers using collapsed attribute
collapsing containers on app start (see comment in directive uiLayoutLoaded)
introduces concept of "central" container
add animation for collapsing
fixes Create CONTRIBUTING.md #129, Window resize resets column sizes #100
few formatting changes

Enables: - programmatically un/collapse containers using collapsed attribute - collapsing containers on app start (see comment in directive uiLayoutLoaded) - introduces concept of "central" container - add animation for collapsing - fixes angular-ui#129, angular-ui#100 - few formatting changes

j--w · 2015-11-04T04:20:44Z

@petrsimon created a plunker to demonstrate these fixes here: http://plnkr.co/edit/u8hJG42cMQDg9KiG86as

Unfortunately a couple of tests are failing, @petrsimon have you had a chance to look at the failing tests?

petrsimon · 2015-11-04T04:24:25Z

Weird. On my system, I see 61 test running, all passing. Can you tell me which ones?
Edit: and thanks for including the link to plunker.

j--w · 2015-11-04T04:31:03Z

It looks like it's the splitMove tests have a look at the log here for more detail: https://travis-ci.org/angular-ui/ui-layout/builds/88887198

petrsimon · 2015-11-04T04:33:16Z

Ah, thanks. I've missed that. I thought it's just the jshint, since it's all passing on my computer.

petrsimon · 2015-11-04T04:36:43Z

I see, it's failing on Firefox 31, I've got it passing on 41. Are we supposed to support browser that old?

j--w · 2015-11-04T04:47:56Z

I'll defer to @SomeKittens for that question but I would assume so as all the other angular-ui stuff on travis seems to be running tests in that version.

petrsimon · 2015-11-04T04:51:07Z

Right. That naturally makes sense. Just not sure how to go about fixing this...

petrsimon · 2015-11-04T05:06:44Z

Just ran the test with FF31.
Firefox 31.0.0 (Mac OS X 10.11.0): Executed 61 of 61 SUCCESS (2.393 secs / 2.212 secs)
Not sure what to do next.

j--w · 2015-11-04T05:23:15Z

I just pulled down your branch to my local and the same tests fail for me in chrome 46 and firefox 41 but pass in phantomjs. Any ideas? I may be able to do a bit of digging tomorrow.

petrsimon · 2015-11-04T09:06:36Z

OK, I can reproduce it now, only on Chrome, though... (on OSX) (confession: actually I didn't have stable chrome installed and I forgot to turn Canary on, that's why I missed it). Thanks a lot and also for the eagerness to dig!

It bugged me, so I had to take a quick look. It's caused by rounding down.

To recap what is going on in the test:

Initially, the available space (minus dividerSize) is divided by the number of 'auto' sized containers, two in our case. See line 311. This number is rounded down, which means that there's 1px border left at the bottom in case of odd numbers. Splitbar top is this rounded down number.
On toggle, this branch stores the actual size of the container in uncollapsedSize property and uses it during the next recalculation, where available size is divided by one (remaining 'auto' sized container), which means that the splitbar top is shifted by 1px up.

The test is working outside this branch, because originally toggle{After,Before} were simply adding/subtracting the original size and leaving the 'auto' container as it was (including the dangling 1px, of course).

How about if in case of odd numbers, we add the remainder to the last 'auto' sized container and adjust the tests.
Does that make sense? @BobbieBarker and @SomeKittens do have any comments?

SomeKittens · 2015-11-04T17:17:41Z

Opened #156 to cover browser testing.

And yeah, rounding is fun! I'm fine with adjusting the tests to cover reasonable off-by-one cases due to rounding.

petrsimon · 2015-11-04T23:03:58Z

Thanks @SomeKittens!
Sorry, my explanation above is perhaps little unclear. I thought this is a browser issue at first, but see my notes on the "dangling pixel". We need to fix that and modify the tests. It should than work on all browsers again. Otherwise the test fails randomly based on the initial size of browser in karma, which in some cases is odd (pun intended).

The current tests are simply testing a bug, which is caused by rounding in the code (not default sub-pixel value rounding in the browsers). Hackish way to fix the test would be to resize the viewport in beforeEach to even numbers.

But I guess the best way to do it is to add the dangling pixel to the last auto sized element.

j--w · 2015-11-06T19:37:35Z

I think adding the dangling pixel to the last auto sized element makes sense.

BobbieBarker · 2015-11-07T21:09:33Z

src/ui-layout.js

+            c = ctrl.containers[i];
+            opts.sizes[i] = c.isCentral ? 'auto' : c.collapsed ? '0px' : optionValue(c.uncollapsedSize) || 'auto';
+            opts.minSizes[i] = optionValue(c.minSize);
+            opts.maxSizes[i] = optionValue(c.maxSize);

            if(opts.sizes[i] != 'auto') {


BobbieBarker · 2015-11-07T21:20:39Z

I'm also fine with adjusting the tests to deal with the scenario. I just did an initial review I found a few style issues. I'm going to try and review it again to dig into more of what you're doing etc before I sign off, but initial review looks good. Should be able to get it merged in pretty quickly after the tests are fixed.

petrsimon · 2015-11-07T22:00:28Z

I didn't want to mess with the style too much, since I've got more ideas in that area and thought I'll do another PR for that, but those few small changes you've mentioned, I'll change right away. I've got the dangling pixel basically taken care of, just have to fix the test and see that it doesn't brake anything else.

SomeKittens · 2015-11-07T22:01:37Z

Yeah, there's a few lingering style issues I'd like to fix but haven't yet. Thanks for following what you saw.

Good news about the pixel! Looking forward to it.

petrsimon · 2015-11-07T22:02:46Z

Will do my best! :)

Remainder from rounding down of autoSize value is added to the last container of a layout. - Tests adjusted to account for the new container sizing - Few style fixes as suggested during PR review

petrsimon · 2015-11-08T01:31:05Z

Please have a look.

SomeKittens · 2015-11-10T00:56:26Z

Latest commit LGTM

petrsimon · 2015-11-10T06:07:40Z

My pleasure! Let's see if others find anything fishy...

BobbieBarker · 2015-11-15T04:27:20Z

LGTM as well, it looks like jshint failed on your build though. Fix and we'll accept. Also, thank you for putting up with all the back and forth, your contribution is definitely welcome!

This is temporary formatting change, which should later be revised.

petrsimon · 2015-11-16T02:04:09Z

No worries, happy to contribute.
I've pushed a temporary fix for the jshint warning, which doesn't reformat the whole file. I would personally consider switching to $inject property or even better to ng-annotate later.

feat: Programmatically toggle containers

BobbieBarker · 2015-11-16T06:22:10Z

PR accepted, this project needs a lot of TLC if you'd like to submit additional pull requests to deal with short comings etc you're more than welcome to.

petrsimon · 2015-11-16T07:18:33Z

Wonderful! Thank you. I'll have couple of other ideas, so I'm planning to make issues or PRs soon.

petrsimon mentioned this pull request Nov 2, 2015

Toggle containers programmatically #152

Closed

BobbieBarker reviewed Nov 7, 2015
View reviewed changes

Add remainder when parent container size is odd

b094eaa

Remainder from rounding down of autoSize value is added to the last container of a layout. - Tests adjusted to account for the new container sizing - Few style fixes as suggested during PR review

petrsimon mentioned this pull request Nov 10, 2015

add hideprev and hideafter to uiLayoutContainer #158

Open

Fix jshint warning

24664b4

This is temporary formatting change, which should later be revised.

BobbieBarker added a commit that referenced this pull request Nov 16, 2015

Merge pull request #154 from petrsimon/pr-toggle-and-resize

e3461bb

feat: Programmatically toggle containers

BobbieBarker merged commit e3461bb into angular-ui:master Nov 16, 2015

petrsimon mentioned this pull request Nov 17, 2015

Dependency injection cleanup #159

Open

feat: Programmatically toggle containers #154

feat: Programmatically toggle containers #154

Uh oh!

Conversation

petrsimon commented Nov 2, 2015

Uh oh!

j--w commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

j--w commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

j--w commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

j--w commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

SomeKittens commented Nov 4, 2015

Uh oh!

petrsimon commented Nov 4, 2015

Uh oh!

j--w commented Nov 6, 2015

Uh oh!

BobbieBarker Nov 7, 2015

Choose a reason for hiding this comment

Uh oh!

BobbieBarker commented Nov 7, 2015

Uh oh!

petrsimon commented Nov 7, 2015

Uh oh!

SomeKittens commented Nov 7, 2015

Uh oh!

petrsimon commented Nov 7, 2015

Uh oh!

petrsimon commented Nov 8, 2015

Uh oh!

SomeKittens commented Nov 10, 2015

Uh oh!

petrsimon commented Nov 10, 2015

Uh oh!

BobbieBarker commented Nov 15, 2015

Uh oh!

petrsimon commented Nov 16, 2015

Uh oh!

BobbieBarker commented Nov 16, 2015

Uh oh!

petrsimon commented Nov 16, 2015

Uh oh!

Uh oh!