Skip to content

fix(ios-text-base): button textAlignment on IOS (UIButton) #8181

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.

Sign up for GitHub

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

Merged
vtrifonov merged 5 commits into from
Jan 21, 2020
Merged

fix(ios-text-base): button textAlignment on IOS (UIButton) #8181

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9e7da1
fix(8151): button textAlignment on IOS (UIButton)
romain20100-rwigo Dec 8, 2019
c3cf2ca
Merge branch 'master' into fix-uibutton-textalignment
dtopuzov Dec 13, 2019
06698b3
Merge branch 'master' into fix-uibutton-textalignment
dtopuzov Jan 6, 2020
0764df2
Merge branch 'master' into fix-uibutton-textalignment
sylann Jan 11, 2020
e5d2b3b
Merge branch 'master' into fix-uibutton-textalignment
vtrifonov Jan 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions nativescript-core/ui/text-base/text-base.ios.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,12 @@ export class TextBase extends TextBaseCommon {
const paragraphStyle = NSMutableParagraphStyle.alloc().init();
paragraphStyle.lineSpacing = this.lineHeight;
// make sure a possible previously set text alignment setting is not lost when line height is specified
paragraphStyle.alignment = (<UITextField | UITextView | UILabel>this.nativeTextViewProtected).textAlignment;
if (this.nativeTextViewProtected instanceof UIButton) {
paragraphStyle.alignment = (<UIButton>this.nativeTextViewProtected).titleLabel.textAlignment;
} else {
paragraphStyle.alignment = (<UITextField | UITextView | UILabel>this.nativeTextViewProtected).textAlignment;
}

if (this.nativeTextViewProtected instanceof UILabel) {
// make sure a possible previously set line break mode is not lost when line height is specified
paragraphStyle.lineBreakMode = this.nativeTextViewProtected.lineBreakMode;
Expand Down Expand Up @@ -192,7 +197,12 @@ export class TextBase extends TextBaseCommon {
const paragraphStyle = NSMutableParagraphStyle.alloc().init();
paragraphStyle.lineSpacing = style.lineHeight;
// make sure a possible previously set text alignment setting is not lost when line height is specified
paragraphStyle.alignment = (<UITextField | UITextView | UILabel>this.nativeTextViewProtected).textAlignment;
if (this.nativeTextViewProtected instanceof UIButton) {
Copy link
Contributor

@vmutafov vmutafov Dec 17, 2019

Choose a reason for hiding this comment

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

Couldn't we extract the logic of this if statement to a separate function or introduce some polymorphic behavior for UIButton as these lines are duplicated thus increasing the maintenance cost?

Copy link
Contributor Author

@sylann sylann Jan 11, 2020

Choose a reason for hiding this comment

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

I'm not sure it's a good idea in that case. It's only repeated twice and there is a lot of context.

Or maybe there is a bigger picture, but I don't see it.

Copy link
Contributor

@vmutafov vmutafov Jan 14, 2020

Choose a reason for hiding this comment

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

Why do you think it's not a good idea and what do you mean by context?
The bigger picture is that, in my opinion, the codebase is starting to have a lot of code duplications and it's getting common to fix a bug at multiple places. To mitigate this problem I think we should be conservative about adding new code duplications and refactor existing ones whenever possible.

Copy link
Contributor Author

@sylann sylann Jan 18, 2020

Choose a reason for hiding this comment

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

If you are talking about rethinking this whole file to avoid even having this if statement, I agree. But moving just this if statement in a function to avoid repeating 2 lines of code, is a perfect example of over abstraction in my opinion.

However, I do want to say that I have little experience with nativescript, since I almost just discovered this project. So I don't pretend to have a genuine opinion in here. I'm just sharing my "general experience" on what looks like a lot of work for a not so important abstraction.

Copy link
Contributor

@vmutafov vmutafov Jan 20, 2020

Choose a reason for hiding this comment

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

Alright then, I'll approve your changes. We'd perhaps refactor this whole file so we'll abstract stuff at a later point. Thank you for the contribution :)

paragraphStyle.alignment = (<UIButton>this.nativeTextViewProtected).titleLabel.textAlignment;
} else {
paragraphStyle.alignment = (<UITextField | UITextView | UILabel>this.nativeTextViewProtected).textAlignment;
}

if (this.nativeTextViewProtected instanceof UILabel) {
// make sure a possible previously set line break mode is not lost when line height is specified
paragraphStyle.lineBreakMode = this.nativeTextViewProtected.lineBreakMode;
Expand Down