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

MaxLines property on Label #3318

Merged
merged 8 commits into from Aug 16, 2018

Conversation

@jfversluis
Copy link
Member

commented Jul 17, 2018

Description of Change

As proposed in #2060, introduced a MaxLines property on a Label. Implemented for:

  • iOS
  • Mac OS
  • Android
  • UWP

Issues Resolved

API Changes

Added:

  • int Label.MaxLines { get; set; } //Bindable Property

Platforms Affected

  • Core/XAML (all platforms)
  • iOS
  • Mac OS
  • Android
  • UWP

Behavioral/Visual Changes

Since this is a new property with a default value as the control has now, the user shouldn't see any changes. When setting the property, the Label will limit the number of lines to the stated value.

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard
jfversluis added 5 commits Jun 6, 2018
Update from base
Merge from origin
Merge from origin
New changes from origin
Introduced new MaxLines property on Label
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

Not sure why the maxlines in the iOS/Mac OS renderer were set when setting the truncation. From what I tested this works together well the way I implemented it now. But a second-opinion is appreciated.

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

Also, if anyone has any idea on how to implement on WPF, GTK and Tizen, I'm open to that.

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Uh-oh. What did I break?

@@ -78,6 +78,8 @@ public class Label : View, IFontElement, ITextElement, ITextAlignmentElement, IL

public static readonly BindableProperty LineHeightProperty = LineHeightElement.LineHeightProperty;

public static readonly BindableProperty MaxLinesProperty = BindableProperty.Create(nameof(MaxLines), typeof(int), typeof(Label), default(int));

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Jul 23, 2018

Member

use -1 has default

This comment has been minimized.

Copy link
@jfversluis

jfversluis Jul 23, 2018

Author Member

Done :)

@samhouts samhouts added this to Ready for Review in Sprint 140 Aug 11, 2018
@samhouts samhouts requested a review from kingces95 Aug 13, 2018
Copy link
Member

left a comment

please handle reverting to default. otherwise 👍

{
Control.SetSingleLine(Element.MaxLines == 1);
Control.SetMaxLines(Element.MaxLines);
}

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

a else is missing. it should handled the case where a MaxLines value is set, then ClearValue()'d, actually reverting to the control default.

if (Element.MaxLines >= 0)
{
textBlock.MaxLines = Element.MaxLines;
}

This comment has been minimized.

Copy link
@StephaneDelcroix

Layout();
#endif
}

This comment has been minimized.

Copy link
@StephaneDelcroix
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

Rookie mistake... Added it now!

Also added >= 0 check on iOS and Mac instead of > 0 because setting the value to 0 sets it to auto-size. Might be nice to include that for our users.

Copy link
Member

left a comment

not sure 1 is the right default...

@@ -78,6 +78,8 @@ public class Label : View, IFontElement, ITextElement, ITextAlignmentElement, IL

public static readonly BindableProperty LineHeightProperty = LineHeightElement.LineHeightProperty;

public static readonly BindableProperty MaxLinesProperty = BindableProperty.Create(nameof(MaxLines), typeof(int), typeof(Label), -1);

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

we probably should InvalidateMeasure in case the property changes.

also, there's some code in OnTextChanged() that rely on some heuristics to find out if the label is single line or not. those heuristics aren't probably valid anymore.

else
{
Control.SetSingleLine(true);
Control.SetMaxLines(1);

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

I'm might be wrong here, but I'm not sure the default is single line labels.

This comment has been minimized.

Copy link
@jfversluis

jfversluis Aug 14, 2018

Author Member

Correct, will change, although I cannot find a conclusive answer on the max lines value

}
else
{
textBlock.MaxLines = 0;

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

0 means 'no maximum', right ?

#if __MOBILE__
Control.Lines = 1;

LayoutSubviews();

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

it probably depends on the LineBreakMode

This comment has been minimized.

Copy link
@jfversluis

jfversluis Aug 14, 2018

Author Member

Default value == 1, as per: https://developer.apple.com/documentation/uikit/uilabel/1620539-numberoflines.

But agreed that using in conjunction with line break mode makes sense. Then 1 for truncation and 0 (which acts as auto) for wrapping?

This comment has been minimized.

Copy link
@StephaneDelcroix

StephaneDelcroix Aug 14, 2018

Member

Then 1 for truncation and 0 (which acts as auto) for wrapping?

that's the way it was, iirc

This comment has been minimized.

Copy link
@jfversluis

jfversluis Aug 14, 2018

Author Member

You're right! 🙂 pfew, have to get back in there. Was a while ago when I drafted this

Update measurement when MaxLines property value changes and updates to default values.
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

Updates again!

Copy link
Member

left a comment

the isSingleLine here

static void OnTextPropertyChanged(BindableObject bindable, object oldvalue, object newvalue)
{
var label = (Label)bindable;
LineBreakMode breakMode = label.LineBreakMode;
bool isVerticallyFixed = (label.Constraint & LayoutConstraint.VerticallyFixed) != 0;
bool isSingleLine = !(breakMode == LineBreakMode.CharacterWrap || breakMode == LineBreakMode.WordWrap);
if (!isVerticallyFixed || !isSingleLine)
((Label)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged);
if (newvalue != null)
((Label)bindable).FormattedText = null;
}
might need to be modified

@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2018

Ah, I should've mentioned. I didn't think so, because that's kind of the same as this logic on iOS:
1ae5df8#diff-381c6c37e826f38028e8841495e4f78eR364

CharacterWrap and WordWrap mean multi-line, else single line. Still seems to apply?

@StephaneDelcroix StephaneDelcroix merged commit 203d4ff into xamarin:master Aug 16, 2018
1 check passed
1 check passed
license/cla All CLA requirements met.
Details
v3.6.0 automation moved this from In Review to Done Aug 16, 2018
Sprint 140 automation moved this from Ready for Review to Done Aug 16, 2018
@StephaneDelcroix

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Thanks for this PR, @jfversluis

@jfversluis jfversluis deleted the jfversluis:feature/issue-2060 branch Aug 16, 2018
@jfversluis

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2018

No problem at all! Happy to contribute 👍
Thank you for all the good feedback and guidance!

PureWeen added a commit that referenced this pull request Aug 28, 2018
* Implemented MaxLines on iOS, Mac OS, Android, UWP and WPF

Introduced new MaxLines property on Label

* Set default value to -1

* Implemented reverting to default value

* More improvements

Update measurement when MaxLines property value changes and updates to default values.

fixes #2060
fixes #1706
@samhouts samhouts added this to the 3.2.0 milestone Sep 11, 2018
@samhouts samhouts removed this from Done in v3.6.0 Sep 11, 2018
@samhouts samhouts added this to Done in v3.2.0 Sep 11, 2018
@samhouts samhouts removed this from the 3.2.0 milestone Sep 12, 2018
@samhouts samhouts added this to Done in v3.6.0 Sep 12, 2018
@samhouts samhouts removed this from Done in v3.2.0 Sep 12, 2018
@samhouts samhouts added this to the 3.3.0 milestone Sep 20, 2018
@samhouts samhouts removed this from Done in v3.6.0 Sep 21, 2018
@samhouts samhouts added this to Done in v3.4.0 Sep 21, 2018
@samhouts samhouts removed this from Done in v3.4.0 Nov 19, 2018
@samhouts samhouts added this to Done in v3.3.0 Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Sprint 140
  
Done
v3.3.0
  
Done
4 participants
You can’t perform that action at this time.