Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimum width and height should work #386

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
4 participants
@adrianknight89
Copy link
Contributor

commented Sep 27, 2016

Description of Change

VisualElement incorrectly sized minimum dimensions due to these lines:

minimum.Height = Math.Min(request.Height, minimum.Height);
minimum.Width = Math.Min(request.Width, minimum.Width);

If request is 100x100 and minimum is 300x300, Math.Min always sets minimum to 100x100; thus, ignoring user-set minimum dimensions. GetSizeRequest() should return the correct size object.

Layout then should account for minimum size request while positioning visual elements.

The unit test is there to make sure setting minimum dimensions does not inadvertently change request.

Bugs Fixed

I have not verified all, but since they are related to minimum dimensions, they might all be fixed.

@dnfclas

This comment has been minimized.

Copy link

commented Sep 27, 2016

Hi @adrianknight89, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@adrianknight89 adrianknight89 changed the title Minheightwidth Minimum width and height should work Sep 27, 2016

@jassmith

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2016

This would present a breaking change to current apps unfortuantely.

@jassmith jassmith closed this Sep 27, 2016

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 27, 2016

@jassmith How do you hope to fix those issues then? Will MinimumWidthRequest and MinimumHeightRequest never work?

IMO, devs should be informed so that they fix any breaking changes.

OR

Can we add a check to look at XF version ID so that new apps use the fixed version? I know this would create clutter in the code base. Just an idea.

@rogihee

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2016

People are clearly expecting a different behavior, looking at all the bugs. Is it not better to do the breaking change in a new point release, say 2.4?

Why reject a fix?

@adrianknight89

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2016

@jassmith I ran 13 different tests on an Android button with my code changes:

image

Only one produces incorrect result. This is because of the way GetSizeRequest works. Instead of measuring once for width/height request and once for minimum width/height request, it measures only once and tries to update minimum. Unfortunately, minimum gets 55.5 units for height instead of 48. More explanation here: https://forums.xamarin.com/discussion/77854/xamarin-forms-2-3-3-pre2/p3 (see second comment)

All unit tests pass.

I'd still test my code on your end unless you have a strong reason (please explain) not to do so. Right now, minimum size request does not work at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.