Closed donnielrt closed 8 years ago
@jeffkamo @donnielrt What's the situation with this PR? @fractaltheory Adding you to the list
@Helen-Mobify it needs QA + eyes before being merged in
@donnielrt Do we have 2 engineer +s yet?
@Helen-Mobify I'm currently testing this on several devices. :ghost:
@Helen-Mobify Sonja's testing it, but we need a sanity check from another dev/eng
@donnielrt @Helen-Mobify I came across a few different issues while testing. I'm not sure if they are meant to be solved in this PR, or are OOS, so please ignore accordingly. :)
iOS 6 - 7.x.x: Focusing on an input causes the pinny to jump around before scrolling the input into view. iOS 6+: The cursor remains fixed on the screen when scrolling after focusing on an input
Everything is looking good on Android and Firefox.
@yourpalsonja The focus input jump is an issue that was fixed awhile back. So we need that fixed before merging in.
The cursor remaining fixed on screen when scrolling is a Safari bug. That is something cannot be done about.
@donnielrt @Helen-Mobify Testing again now, I can't replicate the iOS 6 - 7.x.x "jumpy input" issue. So, we're all good on the "test on all target devices" front.
Just need two +1's to merge!
Just throwing in a note here, IIRC reproducing that jumpy input issue required that the content was NOT overflowing (ie. no scroll bar).
Thanks @ericmuyser , I'll take another look to make sure I didn't miss anything.
ahh @ericmuyser so it's when there is not enough content in pinny to cause scrolling
@ericmuyser @Helen-Mobify I've tried to re-create using those conditions, but still don't see the issue. It appears it was fixed!
@yourpalsonja @donnielrt
:+1: For this PR, as long as that repaint issue is part of the Lockup PR https://github.com/mobify/lockup/pull/11 I tried using current Lockup and I don't see the repaint.
I have tested this Pinny branch and tested the affected code paths.
Definitely :+1: on the blur on input on orientation change
@Helen-Mobify :tada: Does this mean it just needs another +1 to get merged?
Yup! One more from someone else.
@donnielrt @ericmuyser :eyes:
@yourpalsonja @Helen-Mobify thanks for taking this on!
@Helen-Mobify we'll need the repaint flash fixed in the lockup PR before we can merge this one in. Can you show it to me tomorrow?
Sure thing
Hey, we’re looking to prune older abandoned PRs. If this PR is still relevant and you would like to see it merged in, please reopen the PR and we’ll add it to our backlog! Thanks!
@marlowpayne and I were looking at this the other day, don't know if he has anything to add.
@yourpalsonja does that mean we still need the changes mentioned here? If so we can reopen and make this a release!
+1 Code looks good and is working on BTR
@donnielrt I'm not 100% sure for TG, but I remember testing it on BTR and it was working. Let me ask Marlow.
@yourpalsonja @ericmuyser ok, let me know and we'll reopen this!
Back by popular request.
@haroldtreen this release is ready for your PR
@donnielrt Merged stuttered animation into btr-fixes
:dancers:
@haroldtreen @fractaltheory ok, finally got those damn tests in. We should be able to publish tomorrow after I get a +1.
Does this PR also resolve the known issue described in the README? https://github.com/mobify/pinny/blob/master/README.md#known-issues
@fractaltheory I believe the screen still jumps around a bit when focusing on the keyboard for < iOS 7+
Tests are passing for me, example pinnies are doing what I expect on my iPhone 6s iOS 9.0 --- :+1:
:+1:
Status: Ready for merge Reviewers: @ericmuyser @Helen-Mobify @haroldtreen @fractaltheory
Related PRs
https://github.com/mobify/lockup/pull/11
87
TODOs
Fix bug causing all window orientationchange listeners to be unset on closing pinnyChanges
lock
andunlock
events even if not locking/unlocking due to presence of activePinnies