Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

GH-3106 Implemented LineBreakMode to Button #7810

Closed
wants to merge 21 commits into from

Conversation

pictos
Copy link
Contributor

@pictos pictos commented Oct 4, 2019

Description of Change

Added the LineBreakMode to Button.

Issues Resolved

API Changes

Added:

  • LineBreakMode Button.LineBreakMode { get; set; } //Bindable Property

Platforms Affected

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

Behavioral/Visual Changes

Now users can set a line break mode to texts on Button, the default value doesn't affect any user.

None

Before/After Screenshots

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

- Added LineBreakMode in ButtonRenderer
- Fixed button height update in Material iOS Button;
- Code style fix on UWP
Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
internal static class TextBlockExtensions
{
public static void UpdateLineBreakMode(this TextBlock textBlock, LineBreakMode lineBreakMode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@samhouts samhouts moved this from In Review to In Progress in v4.4.0 Oct 7, 2019
@samhouts samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Oct 7, 2019
@pictos
Copy link
Contributor Author

pictos commented Oct 8, 2019

@jsuarezruiz I'll send the Android implementation later in this PR.

@pictos
Copy link
Contributor Author

pictos commented Oct 8, 2019

I realized that Label.LineBreakMode.CharacterWrap on Android doesn't work, can I add the fix in this PR?
#7118

@jsuarezruiz
Copy link
Contributor

Mnn, better separate in another PR to facilitate traceability and history.

Xamarin.Forms.Platform.Android/ButtonLayoutManager.cs Outdated Show resolved Hide resolved
@@ -33,9 +33,9 @@ static void SetMaxLines(this TextView textView, Label label, int lines)
textView.SetMaxLines(lines);
}

public static void SetLineBreakMode(this TextView textView, Label label)
public static void SetLineBreakMode(this TextView textView, Label label, Button button = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ABI breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samhouts even with button being null by default is an API breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's technically not a breaking change yet, but an optional parameter here feels a bit strange. I think I'd rather see two methods here with some code duplication or some refactoring to call a third common method.

@pictos pictos marked this pull request as ready for review October 13, 2019 20:31
@jsuarezruiz
Copy link
Contributor

@pictos Sure, let me check the code to see what may be happening. What platform does it happen (Android or iOS)?. Is it something that always happens?.

Captura de pantalla 2019-10-30 a las 18 01 35

(This screenshot is using CharacterWrap).

@pictos
Copy link
Contributor Author

pictos commented Oct 31, 2019

@jsuarezruiz this occurs when you load the page, as you can see in the gif.
SampleForForms

@samhouts samhouts added this to In Progress in v4.5.0 Nov 2, 2019
@samhouts samhouts removed this from In Progress in v4.4.0 Nov 2, 2019
@pictos
Copy link
Contributor Author

pictos commented Nov 14, 2019

@jsuarezruiz any thoughts?

@pictos
Copy link
Contributor Author

pictos commented Nov 23, 2019

@jsuarezruiz any update on this?

@Happypig375

This comment has been minimized.

Xamarin.Forms.Platform.UAP/ButtonRenderer.cs Outdated Show resolved Hide resolved
Xamarin.Forms.Platform.UAP/LabelRenderer.cs Outdated Show resolved Hide resolved
return;

view.SetLineBreakMode(_element);
view.SetAllCaps(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samhouts, sorry I didn't understand

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we supposed to be forcing AllCaps here? I didn't see this in the original version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samhouts, I'm sorry for the late reply... Well if we don't use that, the text in the button will not follow the actual behavior for text in the Android's button and text will not be all caps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pictos What causes it to not be all caps anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samhouts I don't know why that is, but I can try to find out (if it's needed). Just to exemplify, if I remove this line, the result is:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pictos I think it's work investigating why that is different. Thanks!

@samhouts samhouts added this to In Progress in v4.6.0 Jan 10, 2020
pictos and others added 3 commits January 11, 2020 19:48
Co-Authored-By: Samantha Houts <samhouts@users.noreply.github.com>
Co-Authored-By: Samantha Houts <samhouts@users.noreply.github.com>
Co-Authored-By: Samantha Houts <samhouts@users.noreply.github.com>
@samhouts samhouts removed this from In Progress in v4.5.0 Feb 13, 2020
@samhouts samhouts removed this from In Progress in v4.6.0 Mar 2, 2020
@titoleru
Copy link

Ping - thanks a lot, this will be extremely helpful

@samhouts samhouts added this to In Progress in vCurrent (4.8.0) Mar 19, 2020
@pictos
Copy link
Contributor Author

pictos commented Jun 20, 2020

I will close this one because it this very old, I created the #11147

@pictos pictos closed this Jun 20, 2020
vCurrent (4.8.0) automation moved this from In Progress to Closed Jun 20, 2020
@pictos pictos deleted the fix-3106 branch June 20, 2020 23:49
@samhouts samhouts removed this from Closed in vCurrent (4.8.0) Jul 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] LineBreakMode for Button
5 participants