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

Improved toggle #176

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

Improved toggle #176

wants to merge 19 commits into from

Conversation

widmoser
Copy link
Contributor

@widmoser widmoser commented Jan 7, 2016

This PR simplifies the toggle implementation by refactoring the toggle code into a separate function which can be called programmatically as well as from a click event. As far as I can tell, this allows to remove the code related to uiLayoutLoaded and more importantly the Layout service (which is currently causing a memory leak).

I had to adapt the tests a little, because they were expecting initially "false" values due to the uiLayoutLoaded semantics. Also I removed everything related to ui-layout-loaded including the respective documentation section. If this is considered a too drastic change we can figure out a way to "simulate" the event.

I also tested manually with a layout of three containers and two splitbars and it seems to work quite nicely.

Fixes #175, #128

@SomeKittens
Copy link
Contributor

This is primarily a refactor, so that makes things a bit simpler. How hard would it be to get the tests passing?

@widmoser
Copy link
Contributor Author

widmoser commented Jan 8, 2016

You mean the current tests before the changes @SomeKittens right? I think some of the test simply don't make sense to pass after these changes because they are basically testing that the workaround that was previously in place is working. For example in this line there is a comment indicating that what is being tested for is not what one would expect under normal circumstances. And there are similar things in other tests that I adapted.

The main thing, that needs discussion is ui-layout-loaded. I removed it because the only apparent reason for it being there as far as I understand is the workaround for loading collapsed states, which is also indicated by the comments. Since my PR removes this workaround and the need for this indirection, I thought it would be best to completely remove it in order to simplify the code and make it more robust. But I am aware that I might not have the full understanding of possible other use cases of ui-layout-loaded. Moreover, there is also an event referring to it which some people might be relying on, so I see two options. Either make a "breaking" change or try to "simulate" the behaviour this event was exposing previously.

What do you think @SomeKittens?

@SomeKittens
Copy link
Contributor

Good point, no sense in testing a workaround with a more permanent change. I'd prefer not to create a breaking change with a refactor, so try the simulation.

@widmoser
Copy link
Contributor Author

@SomeKittens: I have simulated the event and marked it deprecated in the documentation.

@umdstu
Copy link

umdstu commented Jun 17, 2016

@widmoser

Trying to see if maybe I copied some code incorrectly or not. I have two columns, and applied this PR. Left column does not have 'size' set; Right column has 'size' set to '235px'. If I click right arrow, the left column takes up most of the whole screen as expected. If I click the left arrow, the column moves to the left, but its width does not update width to auto. if I do not set the initial size of the right column, then clicking the left arrow expands the right column to full width as expected.

So, is default size option 'size' suppose to overwrite the potential expanded size value? Can you reproduce?

Also, noted after posting this, that if you re-size your window with the cursor, and then attempt to expand the left column, the right column hides, but the width of the left column does not change, not does the splitbar move.

Thanks

@pabloes
Copy link

pabloes commented Jun 13, 2017

I'm quite interested in the memory leak problem solved here, but I see last comment is from Jul 2016, is there any plan to merge or solve it (the memory leak) ? Do you need a smaller fix for the memory leak in 1.4.3?

edited: I did a small fix only for the memory leak: #230

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.

4 participants