-
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
Respect safe area for "Reply to post" section on the Comments screen #22628
Conversation
Generated by 🚫 Danger |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22628-d5e6dde | |
Version | 24.3 | |
Bundle ID | com.jetpack.alpha | |
Commit | d5e6dde | |
App Center Build | jetpack-installable-builds #7931 |
|
App Name | ![]() |
|
Configuration | Release-Alpha | |
Build Number | pr22628-d5e6dde | |
Version | 24.3 | |
Bundle ID | org.wordpress.alpha | |
Commit | d5e6dde | |
App Center Build | WPiOS - One-Offs #8901 |
c8a3344
to
ffa6e33
Compare
attribute:NSLayoutAttributeBottom | ||
multiplier:1.0 | ||
constant:0.0]; | ||
self.replyTextViewBottomConstraint.priority = UILayoutPriorityDefaultHigh; |
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.
[Nit] I think the property replyTextViewBottomConstraint
is no longer needed, maybe it should be removed?
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! Thank you!
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.
Left one minor comment. LGTM! 🚀
@@ -364,7 +363,7 @@ - (void)configureKeyboardManager | |||
self.keyboardManager = [[KeyboardDismissHelper alloc] initWithParentView:self.view | |||
scrollView:self.tableView | |||
dismissableControl:self.replyTextView | |||
bottomLayoutConstraint:self.replyTextViewBottomConstraint]; | |||
bottomLayoutConstraint:self.keyboardManager.bottomLayoutConstraint]; |
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.
KeyboardDismissHelper
earlier, but it does seem this type requires a bottomLayoutConstraint
. But we cannot pass its own self.keyboardManager.bottomLayoutConstraint
in the initializer. This can cause crashes and unexpected behavior at runtime:
- The property
KeyboardDismissHelper.bottomLayoutConstraint
is expecting a non-optional value. self.keyboardManager.bottomLayoutConstraint
is not initialized yet, and code like this is not possible in Swift. So although this is allowed in Objective-C, weird behavior can occur at Runtime.
I suggest to bring back the self.replyTextViewBottomConstraint
property and set it to:
self.replyTextViewBottomConstraint = [self.view.keyboardLayoutGuide.topAnchor constraintEqualToAnchor:self.replyTextView.bottomAnchor];
Then activate it alongside other constraints
[NSLayoutConstraint activateConstraints:@[
[self.replyTextView.leadingAnchor constraintEqualToAnchor:self.replyTextView.leadingAnchor],
[self.replyTextView.trailingAnchor constraintEqualToAnchor:self.replyTextView.trailingAnchor],
self.replyTextViewBottomConstraint
]];
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.
Fixes #22603
This PR fixes missing padding for the reply section on the Comments screen that ignored the safe area and was overlayed by the Home indicator.
RPReplay_Final1708452812.MP4
To test:
Reply
section respects the bottom safe areaReply
section is visible above the keyboardRegression Notes
Potential unintended areas of impact
Comments screen.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing, UI tests.
What automated tests I added (or what prevented me from doing so)
Automated tests already exist.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: