FluentLayout / Cirrious.FluentLayout

FluentLayout for Xamarin.iOS - sample uses MvvmCross
Microsoft Public License
148 stars 54 forks source link

Remove parentView is UIScrollView from VerticalStackPanelConstraints #20

Closed munkii closed 7 years ago

munkii commented 9 years ago

I have had some issues recently with AutoLayout of cells with variable height in Tables. The issues are outlined in this SO question, https://stackoverflow.com/questions/30099363/uilabel-not-wrapping-in-uitableview-until-device-rotate-ios8

One of my key takeaways from this was ensuring that i gave AutoLayout enough information to do its job of saying how tall each cell should be. The info that I was forgetting was telling the bottom row of UI to hug or "dock" to the BottomOf the container. You can see the changes made by SharpMobileCode to my GitHub Repro https://github.com/munkii/TableCellResizeIssue/blob/c54a21f322b6ede26dd92346fab4364c1a29b1da/TableCellResizeIssue/Controls/CustomCells/CustomerItemCell.cs

I am currently working with a custom control that vertically stacks a variable number of Labels based on binding to a List of strings. If use FluentLayout's VerticalStackPanelConstraints the labels layout (initially) but overflow into subsequent rows and after scrolling are then clipped.

If I remove the check that the container view is a UIScrollView causing the last item in the "stack" to be constrained to the bottom, always, it works.

I have updated by TableReSizeIssue repro to demonstrate the issue. Check out Lines 90-92, https://github.com/munkii/TableCellResizeIssue/blob/master/TableCellResizeIssue/Controls/StringStackPanel.cs#L90

LayoutManually(), is just me laying out myself LayoutWithFluent(), is a copy of the code from GitHub but I have edited it to remove the UIScrollView check

There is also a version that just uses VerticalStackPanelConstraints to show what happens as is

munkii commented 9 years ago

Any got any thoughts on this one?

slodge commented 9 years ago

I wasn't actually sure what the question was... and still not...

Maybe I'm having a woods and trees moment... or maybe it's just I'm not used to the sun...

What do you want the change in this libraries code?

munkii commented 9 years ago

I am suggesting

"We remove the check that the container view is a UIScrollView causing the last item in the "stack" to be constrained to the bottom, always"

AutoLayout needs as much help as possible so the last item in the list should always been constrained to the bottom. I thought I had done such a great job of explaining myself as well :-)

slodge commented 9 years ago

OK - still lost - still not used to this sunshine...

Maybe just send the pull request with the change?

:)

slodge commented 9 years ago

oops - hit "comment and close" by mistake - fat fingers....

munkii commented 9 years ago

OK I will. Still getting used to this Github shenanigans

gshackles commented 8 years ago

Closing this due to inactivity, please reopen if any action is still required here

munkii commented 8 years ago

I have updated my repo of my issue (The previous version was just not obvious enough or easy enough to run) https://github.com/munkii/TableCellResizeIssue

Essentially I believe that the line

if (parentView is UIScrollView)

https://github.com/FluentLayout/Cirrious.FluentLayout/blob/17e050f4f9e943c93ee60044423f421ec5449eb0/Cirrious.FluentLayout/AdvancedFluentLayoutExtensions.cs#L133

Should be removed from VerticalStackPanelConstraints as I always want the BottomConstraint added whether or not the parent is a UIScrollView. However seeing as you guys have recently done a lot of work on Fluent I must be mad as surely you would have seen this issue too.

My TableCellResizeIssue repo shows the problem pretty clearly

Thanks

p.s. I don't think I can reopen, only comment

gshackles commented 8 years ago

Thanks for the repro, will have to check that out. Without having really looked into it yet, I'll say that I'd be hesitant to change the behavior here given that it could impact anyone already using it the way it is. Perhaps in the meantime we could at least add another optional parameter to that method to prevent that constraint from being created, as much as I'd like to avoid that.

munkii commented 8 years ago

I appreciate the concern about changing it and I also agree that an optional param is unfortunate . My worry is that the problem has only occurred due to a mistake from me but I don't think so

Sent from Outlook Mobile

On Mon, Mar 21, 2016 at 12:19 PM -0700, "Greg Shackles" notifications@github.com wrote:

Thanks for the repro, will have to check that out. Without having really looked into it yet, I'll say that I'd be hesitant to change the behavior here given that it could impact anyone already using it the way it is. Perhaps in the meantime we could at least add another optional parameter to that method to prevent that constraint from being created, as much as I'd like to avoid that.

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub

xleon commented 8 years ago

The question is if sticking to the bottom will be always needed. I´m not really sure as I would have to make some testing. If that´s the case I would just remove that line. If not, a one liner workaround seams obvious:

View.AddConstraints(View.AdvancedVerticalStackPanelConstraints(...));
View.AddConstraints(lastItem.AtBottomOf(View));
gshackles commented 7 years ago

Going to close this one out since it's been awhile, but this will be good to keep in mind if/when we get around to a major version bump where we can consider making breaking changes!

munkii commented 7 years ago

ok