Fix placeholder position for RTL layout#591
Conversation
diegoreymendez
left a comment
There was a problem hiding this comment.
The feature in itself is working good, but I added some comments on the code side that would (IMHO) make the code slightly cleaner and easier to read and maintain.
| return contentInset.left + textContainerInset.left + textContainer.lineFragmentPadding | ||
| } | ||
|
|
||
| var isRTLLayout: Bool { |
There was a problem hiding this comment.
This is pretty small, but considering this is a property of RCTAztecView, a more precise naming for this property could be hasRTLLayout.
|
|
||
| override func layoutSubviews() { | ||
| super.layoutSubviews() | ||
| fixLabelPositionForRTLLayout() |
There was a problem hiding this comment.
This call here has me a bit confused because of the asymmetry between how we handle LTR and RTL: for LTR we have a fixed constraint setup at view-initialization time (well almost because it's lazy, but I digress), whereas for RTL we can update it dynamically whenever layoutSubviews is called.
In my mind if we need a dynamic solution, we need both updates here, whereas if a static solution is fine, we could move this code to the lazy initializer of placeholderHorizontalConstraint and make it something like:
private lazy var placeholderHorizontalConstraint: NSLayoutConstraint = {
return placeholderPreferedHorizontalAnchor.constraint(
equalTo: leftAnchor,
constant: isRTLLayout ? rightTextInset : leftTextInset
)
}()Does that make sense? Let me know if there's more to it.
There was a problem hiding this comment.
This asymmetry is actually needed because of the horizontal flipping of the scroll views on RTL layout.
When we try to add a view on a horizontally flipped view, what happens is that the left and right right sides are also flipped logically, but RN flips back the child views so they don't look reversed.
When we try to constraint the placeholder's right anchor to the parent's right anchor (as we would normally do) this happens:
Logically it's correctly anchoring it to the right, but it's visual positioned at the left. You can see the long constraint across the Header, that it's set to 0, and breaks constraints ¯_(ツ)_/¯
That's why we set the right anchor of the left anchor of the parent, that will give us this:
This is not perfect yet, but now the horizontal constraint will respond properly, so we can work with it.
What we have to do is to move the label all the way to the other side of the screen. That's when the dynamic value comes to play.
We set the constant of the constraint to be the width of the parent, and if the parent changes, this has to update accordingly. That's why it's in layoutSubviews.
In the other hand, for LTR, a static value will be enough, since constraining left to left anchors won't need modifications if the layout changes.
I hope this makes clearer.
If we just want consistency, we can always update "dynamically" on both RTL and LTR layouts, but LTR is not necessary, and I think it is good to remark this difference.
I see that my code comments weren't clear enough 😅
|
|
||
| private var previousContentSize: CGSize = .zero | ||
|
|
||
| var textHorizontalInset: CGFloat { |
There was a problem hiding this comment.
A suggestion for clarity / readability / maintainability:
var leftTextInset: CGFloat {
return contentInset.left + textContainerInset.left + textContainer.lineFragmentPadding
}
var rightTextInset: CGFloat {
return bounds.width - leftTextInset
}There was a problem hiding this comment.
Actually, the naming is off... the second one should be leftTextInsetInRTLMode or something like that.
There was a problem hiding this comment.
I mean, naming aside my suggestion would be to have a calculated property for both options.
diegoreymendez
left a comment
There was a problem hiding this comment.
Thanks for the clarification. Let's go ahead and merge this, since it's working as advertised.
Changed to `leftTextInset` and created an RTL analog: leftTextInsetInRTLLayout
|
Thank you @diegoreymendez for the review! |


This PR fixes an issue on RTL layout with the position of placeholders:
iOS doesn't manage RTL layout automatically on scroll views, so React Native will flip horizontally RCTScrollViews on RTL layout to get the Right to Left effect.
The problem is that we are inserting an view into an already flipped tree of views, and this created all sort of weird layout behavior, braking the

leadingandtrailinganchored constraints:Note:
transform = [-1, 0, 0, 1, 0, 0];The solution is a non-conventional horizontal constraint on RTL layout, since
rn-aztec's left is logically at the right and right is at the left, but visually they are positioned "correctly.Since this solution is non-conventional, I added plenty of explanation in the code itself, please let me know if it is clear enough.
The result of this is a correct placement of the placeholder at the right:

(Tested with
right-to-left pseudolanguage)To test:
rake dependenciesoptions ⌥key, click on thePlay (build and run)button on Xcode.Right-To-Left Pseudolanguage