Skip to content

Fix placeholder position for RTL layout#591

Merged
etoledom merged 3 commits intodevelopfrom
issue/rtl-placeholder
Feb 15, 2019
Merged

Fix placeholder position for RTL layout#591
etoledom merged 3 commits intodevelopfrom
issue/rtl-placeholder

Conversation

@etoledom
Copy link
Copy Markdown
Contributor

@etoledom etoledom commented Feb 12, 2019

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 leading and trailing anchored constraints:
screen shot 2019-02-12 at 7 55 09 pm

<RCTCustomScrollView: 0x7f99a3839c00; baseClass = UIScrollView; frame = (0 0; 375 646); transform = [-1, 0, 0, 1, 0, 0]; 

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)
rtl-placeholder

To test:

  • Checkout the corresponding WPiOS PR
  • rake dependencies
  • Holding options ⌥ key, click on the Play (build and run) button on Xcode.
  • On Run -> Options -> Application Language, choose: Right-To-Left Pseudolanguage
  • Run the app.
  • Do your best to open an instance of Gutenberg with everything flipped :)
  • Check that the Aztec placeholders show properly as in the attached gif.

Copy link
Copy Markdown
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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:

screen shot 2019-02-15 at 6 20 45 pm

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:

screen shot 2019-02-15 at 7 07 24 pm

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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A suggestion for clarity / readability / maintainability:

var leftTextInset: CGFloat {
    return contentInset.left + textContainerInset.left + textContainer.lineFragmentPadding
}

var rightTextInset: CGFloat {
    return bounds.width - leftTextInset
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, the naming is off... the second one should be leftTextInsetInRTLMode or something like that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean, naming aside my suggestion would be to have a calculated property for both options.

Copy link
Copy Markdown
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

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
@etoledom
Copy link
Copy Markdown
Contributor Author

Thank you @diegoreymendez for the review!
I updated with two of the suggested changes, now I'll merge when ✅

@etoledom etoledom merged commit a4390b0 into develop Feb 15, 2019
@etoledom etoledom deleted the issue/rtl-placeholder branch February 15, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants