Skip to content
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

Implement the shadow node for proper laying out Aztec on Android #1093

Merged
merged 3 commits into from Jun 10, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Jun 7, 2019

Addresses #1091

This PR implements the shadow node logic for the Aztec wrapper custom React Native view. This is needed for better View calculations.

This work begun as an attempt to remove the need to specify the minHeight style on the Aztec wrapper. I think that the view should be able to update its height automatically within RN and the Android layout system, without us manually having to set it as a style prop. In that respect, the effort here is not finished; the minHeight style is still needed (try removing it and you'll see that the wrapper doesn't update its height properly, although its now initial height is much better than before). Although, my experiments so far with a basic demo app (in repo: https://github.com/hypest/RN060-aztec-experiments) seem to work and the Aztec wrapper there seems fine without specifying any minHeight.

Even in this state, the behavior in this PR is better than before and that's why I'm opening this PR.

Known issue: the height of the component seems smaller than the content text but only by a little. The onContentSizeChange logic repairs it but it's still visible. Besides, the glitch is minimized due to the work on #1076 and that's why this PR builds on top of that, albeit in a separate PR so #1076 doesn't get blocked/complex.

To test:

  1. Run the demo app on Android
  2. Put the caret in the middle or the end of the "Lorem ipsum" paragraph block
  3. Split the block
  4. Notice that the visual glitch of the existing block resizing is quite small

I have also
Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@hypest hypest added this to the v1.7 milestone Jun 7, 2019
@marecar3
Copy link
Contributor

marecar3 commented Jun 7, 2019

Hey @hypest, I tested this branch a little bit and it seems that it's an improvement 👍

@marecar3 marecar3 self-requested a review June 7, 2019 16:13
@hypest hypest changed the base branch from issue/1029-keyb-on-focus to develop June 8, 2019 00:11
Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

❤️ Way better than before. LGTM!

if (extraData instanceof ReactTextUpdate) {
ReactTextUpdate update = (ReactTextUpdate) extraData;

view.setPadding(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting this warning in AS about these lines for RTL compatibility:

For RtL compatibility, use setPaddingRelativeor ViewCompat.setPaddingRelative() when setting left/right padding. less... (Ctrl+F1) 
Inspection info:For RtL compatibility, use setPaddingRelativeor ViewCompat.setPaddingRelative() when setting left/right padding.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a local linting issue on my end.. I couldn't determine why it gave this warning when ReactTextUpdate merely inherits from java.lang.Object 🤔

Copy link
Contributor Author

@hypest hypest Jun 10, 2019

Choose a reason for hiding this comment

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

I'm getting this warning in AS about these lines for RTL compatibility:

Good point. I've been actually mirroring the code in upstream's implementation of the TextInput shadow node here: https://github.com/facebook/react-native/blob/d88e4701fc46b028861ddcfa3e6ffb141b3ede3d/ReactAndroid/src/main/java/com/facebook/react/views/textinput/ReactTextInputManager.java#L193-L197

I'm kinda torn but, I think it makes more sense at the moment to follow upstream's lead on this one. Alternatively, we can even completely remove the code to set the padding since it's not a real goal for this PR anyway. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it makes more sense at the moment to follow upstream's lead on this one.

I agree, this seems like the best way forward. I haven't tested many scenarios without the code (only the "Lorem ipsum" test in the description), and I'm unsure if there are scenarios where it would be important to have it. So keeping it close to upstream seems the sensible approach. LGTM!

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

I tested these changes and it's very smooth now! 😃

@hypest hypest merged commit 517a602 into develop Jun 10, 2019
@hypest hypest deleted the issue/1029-keyb-on-focus-plus-aztec-shadow-node branch June 10, 2019 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants