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

[iOS/Win] Label will not unnecessarily expand #827

Merged
merged 3 commits into from Mar 22, 2017

Conversation

Projects
None yet
4 participants
@samhouts
Member

samhouts commented Mar 20, 2017

Description of Change

Labels will no longer expand to the width constraint unless required to do so. Regression caused by #529. Jason tried to warn me. What could go wrong, I said. It's just a small fix, I said.

Bugs Fixed

API Changes

None.

Behavioral Changes

This shouldn't break anything, I said.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

samhouts added some commits Mar 20, 2017

@@ -72,15 +72,31 @@ public override SizeRequest GetDesiredSize(double widthConstraint, double height
_perfectSizeValid = true;
}
if (widthConstraint >= _perfectSize.Request.Width && heightConstraint >= _perfectSize.Request.Height)
var widthFits = widthConstraint >= _perfectSize.Request.Width;

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

This bug did not affect Windows, but the logic here was copied from iOS, so I copied the fix over for housekeeping purposes.

@samhouts

samhouts Mar 20, 2017

Member

This bug did not affect Windows, but the logic here was copied from iOS, so I copied the fix over for housekeeping purposes.

var widthFits = widthConstraint >= _perfectSize.Request.Width;
var heightFits = heightConstraint >= _perfectSize.Request.Height;
if (widthFits && heightFits)

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

Refactored, but the logic is the same.

@samhouts

samhouts Mar 20, 2017

Member

Refactored, but the logic is the same.

result.Minimum = new Size(Math.Min(10, result.Request.Width), result.Request.Height);
if (Element.LineBreakMode != LineBreakMode.NoWrap)
var tinyWidth = Math.Min(10, result.Request.Width);
result.Minimum = new Size(tinyWidth, result.Request.Height);

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

More refactoring, no change here.

@samhouts

samhouts Mar 20, 2017

Member

More refactoring, no change here.

var tinyWidth = Math.Min(10, result.Request.Width);
result.Minimum = new Size(tinyWidth, result.Request.Height);
if (widthFits || Element.LineBreakMode == LineBreakMode.NoWrap)

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

This is the change that fixes this bug. Don't continue on to the "expand" logic if the text already fits the container.

@samhouts

samhouts Mar 20, 2017

Member

This is the change that fixes this bug. Don't continue on to the "expand" logic if the text already fits the container.

{
var expandedWidth = Math.Max(tinyWidth, widthConstraint);
result.Request = new Size(expandedWidth, result.Request.Height);
}

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

The rest of these changes are all refactoring for readability.

@samhouts

samhouts Mar 20, 2017

Member

The rest of these changes are all refactoring for readability.

if (widthFits || Element.LineBreakMode == LineBreakMode.NoWrap)
return result;
bool containerIsNotInfinitelyWide = !double.IsInfinity(widthConstraint);

This comment has been minimized.

@samhouts

samhouts Mar 20, 2017

Member

Simplification of if (!double.IsInfinity(result.Request.Width) && !double.IsInfinity(widthConstraint)).

@samhouts

samhouts Mar 20, 2017

Member

Simplification of if (!double.IsInfinity(result.Request.Width) && !double.IsInfinity(widthConstraint)).

@rmarinho rmarinho requested a review from jassmith Mar 21, 2017

@jassmith

Even though I remain super skeptical I can't find anything wrong.

@rmarinho rmarinho merged commit c65a9a8 into master Mar 22, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3749, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests p…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests pa…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests pa…
Details

rmarinho added a commit that referenced this pull request Mar 22, 2017

[iOS/Win] Label will not unnecessarily expand (#827)
* Add repro for 53362

* [iOS] Label will not unnecessarily expand

* [Win] Label will not unnecessarily expand

@samhouts samhouts deleted the fix-bugzilla52263 branch Apr 19, 2017

@samhouts samhouts added D15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestone: 3.1.0 Jun 1, 2018

@samhouts samhouts added this to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts modified the milestones: 2.3.0, 2.3.4 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment