Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2897753002:
Makes settings icon visible when animation completes. (Closed)
|
Created:
3 years, 7 months ago by csashi Modified:
3 years, 7 months ago Reviewers:
Ted C CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMakes settings icon visible when animation completes.
Fixes hintWidth calculation when we reverse animation.
BUG=724624
Review-Url: https://codereview.chromium.org/2897753002
Cr-Commit-Position: refs/heads/master@{#473965}
Committed: https://chromium.googlesource.com/chromium/src/+/5b17d3a93fe37f7f62b9a70acc1fa0cac793e68b
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fade-in views beyond the separatorPosition. #
Total comments: 8
Patch Set 3 : Reduces alpha animation duration. Make settings icon transparent at beginning of animation. #
Messages
Total messages: 21 (13 generated)
csashi@google.com changed reviewers: + tedchoc@chromium.org
Hi Ted, With reference to your comment in https://codereview.chromium.org/2874933008/diff/280001/components/autofill/an... I made a change so that the settings icon does not have a jarring movement at the end of the animation. To my non-visual-designer eye, making the view visible at the end of the animation seems better. With this change: https://drive.google.com/open?id=0B-jTnU8uuAL9WTRXZWgtaWF4WlE Without this change (I had incorrectly added the padding in the hintWidth computation too). https://drive.google.com/corp/drive/u/0/folders/0B-jTnU8uuAL9WEJvYzBNZnozWWs Please take a look. Thanks! -sashi.
Description was changed from ========== Makes settings icon visible when animation completes. BUG=724624 ========== to ========== Makes settings icon visible when animation completes. Fixes hintWidth calculation when we reverse animation. BUG=724624 ==========
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2897753002/diff/1/components/autofill/android... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java (right): https://codereview.chromium.org/2897753002/diff/1/components/autofill/android... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:290: mAnimator.addListener(new AnimatorListenerAdapter() { Hmm...I still think it is pretty jarring to have the settings icon pop in. I think it would be best to set it to INVISIBLE in the block below and have a sequential animation (https://developer.android.com/reference/android/animation/AnimatorSet.html#pl...) where you animate the alpha of the settings icon from transparent to visible. That fade "should" make it seem intentional.
Hi Ted, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2897753002/diff/1/components/autofill/android... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java (right): https://codereview.chromium.org/2897753002/diff/1/components/autofill/android... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:290: mAnimator.addListener(new AnimatorListenerAdapter() { On 2017/05/22 21:39:46, Ted C wrote: > Hmm...I still think it is pretty jarring to have the settings icon pop in. > > I think it would be best to set it to INVISIBLE in the block below and have a > sequential animation > (https://developer.android.com/reference/android/animation/AnimatorSet.html#pl...) > where you animate the alpha of the settings icon from transparent to visible. > That fade "should" make it seem intentional. Done. I had previously considered fade-in animations but was concerned that it may call out attention to the icon which is likely to have low usage. But agreed that it makes it smoother. I couldn't keep the view as INVISIBLE - looks like animation has no effect on them then. So I marked it as 100% transparent to start and then animated to fully opaque. New video @: https://drive.google.com/open?id=0B-jTnU8uuAL9cDUyYXJoTVVucFk
https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java (right): https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:43: private static final long ALPHA_ANIMATION_DURATION_MILLIS = 1000; I would set this somewhere around 200-250ms. 1sec is a pretty long animation and will draw more attention to the settings (we try to keep most of our animations in chrome in the 2-300ms) https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:207: mAnimator.addListener(new AnimatorListenerAdapter() { should we set the trailing visible to transparent in this block? I think it would be good to isolate all the animation in this block and the method below to make it consistent between the two. for (int i = mSeparatorPosition + 1; i < getChildCount(); i++) { getChildAt(i).setAlpha(0); } https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:308: mAnimator = ObjectAnimator.ofFloat(this, View.TRANSLATION_X, 0, -hintWidth); You can change the type of mAnimator to just be plain Animator. Then you can create an AnimatorSet here to avoid creating it in the runnable below, which makes it a bit clearer. https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:326: alphaAnimators.add(ObjectAnimator.ofFloat(getChildAt(i), View.ALPHA, 0, 1.0f)); If you can avoid setting the initial value, canceling and starting new animations will look better (by setting 0 as the third param, you are forcing the value to jump to that as part of this animation, even if you canceled a previous animation that set it to .5 at the end, now it will jump hard between these values). Ideally here and else where you would just do the param version which (getChildAt(i), View.ALPHA, 1f)
Hi Ted, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java (right): https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:43: private static final long ALPHA_ANIMATION_DURATION_MILLIS = 1000; On 2017/05/23 14:15:37, Ted C wrote: > I would set this somewhere around 200-250ms. 1sec is a pretty long animation > and will draw more attention to the settings (we try to keep most of our > animations in chrome in the 2-300ms) Done. https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:207: mAnimator.addListener(new AnimatorListenerAdapter() { On 2017/05/23 14:15:37, Ted C wrote: > should we set the trailing visible to transparent in this block? I think it > would be good to isolate all the animation in this block and the method below to > make it consistent between the two. > > for (int i = mSeparatorPosition + 1; i < getChildCount(); i++) { > getChildAt(i).setAlpha(0); > } Done. https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:308: mAnimator = ObjectAnimator.ofFloat(this, View.TRANSLATION_X, 0, -hintWidth); On 2017/05/23 14:15:37, Ted C wrote: > You can change the type of mAnimator to just be plain Animator. Then you can > create an AnimatorSet here to avoid creating it in the runnable below, which > makes it a bit clearer. Done. https://codereview.chromium.org/2897753002/diff/20001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillKeyboardAccessory.java:326: alphaAnimators.add(ObjectAnimator.ofFloat(getChildAt(i), View.ALPHA, 0, 1.0f)); On 2017/05/23 14:15:37, Ted C wrote: > If you can avoid setting the initial value, canceling and starting new > animations will look better (by setting 0 as the third param, you are forcing > the value to jump to that as part of this animation, even if you canceled a > previous animation that set it to .5 at the end, now it will jump hard between > these values). Ideally here and else where you would just do the param version > which (getChildAt(i), View.ALPHA, 1f) Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by csashi@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495561080348620,
"parent_rev": "23912c2c5b51036910859aa8db07db2a86649d77", "commit_rev":
"5b17d3a93fe37f7f62b9a70acc1fa0cac793e68b"}
Message was sent while issue was closed.
Description was changed from ========== Makes settings icon visible when animation completes. Fixes hintWidth calculation when we reverse animation. BUG=724624 ========== to ========== Makes settings icon visible when animation completes. Fixes hintWidth calculation when we reverse animation. BUG=724624 Review-Url: https://codereview.chromium.org/2897753002 Cr-Commit-Position: refs/heads/master@{#473965} Committed: https://chromium.googlesource.com/chromium/src/+/5b17d3a93fe37f7f62b9a70acc1f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5b17d3a93fe37f7f62b9a70acc1f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
