-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Align last line of skeleton view according to text alignment #431
Conversation
@@ -43,6 +43,10 @@ extension UITextView: ContainsMultilineText { | |||
var numberOfLines: Int { | |||
-1 | |||
} | |||
|
|||
var textAlignment: NSTextAlignment { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to know, why do you provide a default value for the text views? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! I'll remove this 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing 👏🏼
I've checked the code and looks great. I just leave a small comment.
frame = flipRectForRTLIfNeeded(newFrame, isRTL: isRTL) | ||
|
||
// Align last line according to layer text alignment if not RTL | ||
if !isRTL && index == totalLines - 1 && totalLines != 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Juanpe
I'm not sure this is the correct approach. Don't you think that textAlignment
should be a priority regarding the position of the rectangles? If we define the alignment as left
, right
, center
, it should be so. But if the alignment is natural
, then it should reflect the current RTL
state. (reference)
I don't know if this would bring any undesired friction to what you already have and the RTL feature itself... So please let me know what you think would be best! 🙇🏼♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mm...good question 🤔 TBH, I think we should prioritize RTL for natural
alignments for now. So, we could leave this code there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should prioritize RTL for natural
alignments, but this means that we shouldn't for left
, right
, and center
alignments! My suggestion would be to bundle flipRectForRTLIfNeeded
and alignLastLineRect
in a function that would check the alignment and then if natural
, check RTL. This function would return the final frame.
Having two functions that can do the same looks quite confusing and might cause bugs eventually 🐛 💥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it might make sense. In fact, I'm thinking about how the flipRect...
method applies to all lines and it'd really make sense to apply it to only the last line as well.
Any suggestion for the name of this method? maybe transformFrameBasedOnTextAlignment
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we could make it more efficient by only applying it to the last line. I was thinking on bundling up RTL in the function so I guess that name wouldn't be 100% accurate... Perhaps simply
func alignLayerFrame(_ rect: CGRect, alignment: NSTextAlignment, isRTL: Bool) -> CGRect
...since its called from updateLayerFrame
🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM :)
🚀 Thanks for contributing! 🤙 |
Congratulations! 🎉 This was released as part of SkeletonView 1.22.0 🚀 |
Summary
As mentioned in issue 428, it would be nice to have the possibility to align the last line of multiline skeleton views (when not filling 100%) according to their text alignment property. This pull request proposes a solution where given the property
textAlignment
, the rectangle that represents the last line of a multiline layer is positioned accordingly. Only.center
and.right
alignments are taken into account, the fallback will always be to align left.Requirements (place an
x
in each of the[ ]
)