Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2503543003:
[Payments] Add text when shopping card total is updated. (Closed)
Description[Payments] Add text when shopping card total is updated.
BUG=662503
Committed: https://crrev.com/0d9dfefb340989c2cb8655bfbc3f6dd83d352c10
Cr-Commit-Position: refs/heads/master@{#433259}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed comments + added fade out counter #
Total comments: 24
Patch Set 3 : Addressed tchoc's comments #
Total comments: 16
Patch Set 4 : Addressed comments #
Total comments: 4
Patch Set 5 : Fixed Comments #
Messages
Total messages: 25 (11 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:282: final Animation in = new AlphaAnimation(0.0f, 1.0f); No need to mark this final, from what I can see. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:288: new Handler().postDelayed(new Runnable() { Use a member instance mHandler instead. Object creation is expensive on Android. This animations fires every time the user changes their shipping address or option, which can happen multiple times. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:291: final Animation out = new AlphaAnimation(1.0f, 0.0f); Also no need to mark this final. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:430: mUpdatedView.setTextColor(Color.parseColor("#008043")); Please put this color in an XML file. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:437: mSummaryLayout.addView(mUpdatedView, 1, updatedLayoutParams); 1 is a magic constant here. Magic constants lead to bugs. Please put it into a well-named constant. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:653: addUpdateText("Updated"); String should be internationalized. Put it in anrdroid_stirngs.grd or something.
Patchset #2 (id:80001) has been deleted
Thanks, another look? https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:282: final Animation in = new AlphaAnimation(0.0f, 1.0f); On 2016/11/15 18:57:32, rouslan wrote: > No need to mark this final, from what I can see. Done. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:288: new Handler().postDelayed(new Runnable() { On 2016/11/15 18:57:32, rouslan wrote: > Use a member instance mHandler instead. Object creation is expensive on Android. > This animations fires every time the user changes their shipping address or > option, which can happen multiple times. Done. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:291: final Animation out = new AlphaAnimation(1.0f, 0.0f); On 2016/11/15 18:57:32, rouslan wrote: > Also no need to mark this final. Done. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:430: mUpdatedView.setTextColor(Color.parseColor("#008043")); On 2016/11/15 18:57:32, rouslan wrote: > Please put this color in an XML file. Done. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:437: mSummaryLayout.addView(mUpdatedView, 1, updatedLayoutParams); On 2016/11/15 18:57:32, rouslan wrote: > 1 is a magic constant here. Magic constants lead to bugs. Please put it into a > well-named constant. Done. https://codereview.chromium.org/2503543003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:653: addUpdateText("Updated"); On 2016/11/15 18:57:32, rouslan wrote: > String should be internationalized. Put it in anrdroid_stirngs.grd or something. Done.
rouslan@chromium.org changed reviewers: + tedchoc@chromium.org
lgtm on my side. please get a review from an Android UI expert as well. +tedchoc@.
cc'ing dfalcatara about my createMainSectionContent suggestion below https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:195: <color name="payments_updated_text_color">#008043</color> can we just use google_green_700 here instead of defining a new color? we should really, really try to unify our color palette instead of having subtly different colors. They are so nominally different that I ask we use it anyway and I'll push back on UX if they have issues. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:265: * @param rightText Text to display on the right side. Instead of "Text to display", I would say the "Text displayed". I was incorrectly thinking this was the update text itself, but reading it more closely, it is the text such as cost (or whatever) where we want to tell the user it updated. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:267: public void showUpdateIfTextChanged(@Nullable CharSequence rightText) { looks like this could be private as well? https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:276: if (!mSummaryRightTextView.getText().toString().equals(rightText.toString()) I would use TextUtils.equals(...) here https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:282: /* add another * to make this a javadoc. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:286: Animation in = new AlphaAnimation(0.0f, 1.0f); could we use mUpdateView.getAlpha() instead of 0f? Same in the fade out logic below. That way if you were in the middle of fading out, then a fade in would start at the correct value. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:289: mUpdatedView.setAlpha(1.0f); why the need to set the alpha to 1 here? https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:293: mHandler.postDelayed(new Runnable() { If you were to add an AnimationListener to the Animation, would onAnimationEnd be called when you are overwriting the animations from subsequent calls? https://developer.android.com/reference/android/view/animation/Animation.Anim... In particular, that would be nicer than posting tasks. If the listener doesn't work, you could pull out a Runnable. private Runnable mFadeOutRunnable = new Runnable() {...}; Then you could do: mHandler.removeCallbacks(mFadeOutRunnable); mHandler.postDelayed(mFadeOutRunnable, UPDATE_TEXT_VISIBILITY_DURATION_MS); Then you wouldn't need the pendingAnimationCount. The Handler also poses an issue as it will run after this class has gone away, so you could be posting tasks that are run when the view is detached from the view hierarchy, so it is another reason to use the listener if you can. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:426: public void addUpdateText(String text) { all the other methods that do the same seem to be private. any reason for this to be public? https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:427: mUpdatedView = new TextView(getContext()); maybe add an assert that mUpdatedView == null before this? https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:662: addUpdateText(updatedText); dfalcantara@ can probably better speak to this than me, but it looks like this should be done in createMainSectionContent. Then showUpdateIfTextChanged would exist within this class. To me, it seems similar to ExtraTextsSection and it's exposed setExtraTexts stuff.
dfalcantara@chromium.org changed reviewers: + dfalcantara@chromium.org
https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:662: addUpdateText(updatedText); On 2016/11/16 19:29:40, Ted C wrote: > dfalcantara@ can probably better speak to this than me, but it looks like this > should be done in createMainSectionContent. > > Then showUpdateIfTextChanged would exist within this class. To me, it seems > similar to ExtraTextsSection and it's exposed setExtraTexts stuff. Yeah, this belongs in createMainSectionContent with the other controls that are created for this section. If you don't put it there, it gets increasingly harder to determine the order that controls will be added to the layout.
Patchset #3 (id:120001) has been deleted
thanks tchoc, could you please take another look? Thanks https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/re... File chrome/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/re... chrome/android/java/res/values/colors.xml:195: <color name="payments_updated_text_color">#008043</color> On 2016/11/16 19:29:40, Ted C wrote: > can we just use google_green_700 here instead of defining a new color? we > should really, really try to unify our color palette instead of having subtly > different colors. > > They are so nominally different that I ask we use it anyway and I'll push back > on UX if they have issues. Done. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:265: * @param rightText Text to display on the right side. On 2016/11/16 19:29:40, Ted C wrote: > Instead of "Text to display", I would say the "Text displayed". I was > incorrectly thinking this was the update text itself, but reading it more > closely, it is the text such as cost (or whatever) where we want to tell the > user it updated. Oh I see the misunderstanding. But I don't think "Text displayed" works either because it is not the text that is currently displayed. It's the text that will be displayed next. Is "The new cart total that will replace the one currently displayed." better? https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:267: public void showUpdateIfTextChanged(@Nullable CharSequence rightText) { On 2016/11/16 19:29:40, Ted C wrote: > looks like this could be private as well? It has to be public to be available to the nested classes :S Edit: It can now be private in the child class. thanks! https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:276: if (!mSummaryRightTextView.getText().toString().equals(rightText.toString()) On 2016/11/16 19:29:40, Ted C wrote: > I would use TextUtils.equals(...) here Done. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:282: /* On 2016/11/16 19:29:40, Ted C wrote: > add another * to make this a javadoc. Done. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:286: Animation in = new AlphaAnimation(0.0f, 1.0f); On 2016/11/16 19:29:40, Ted C wrote: > could we use mUpdateView.getAlpha() instead of 0f? > > Same in the fade out logic below. > > That way if you were in the middle of fading out, then a fade in would start at > the correct value. I really agree for the fade out. But I'm not sure for the fade in. I kinda like that it starts over if you make a change while the text is still there. Makes it feel reactive to me, that it acknoledges the change I just made. That being said, I'm no UI expert at all so I'll defer to you. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:289: mUpdatedView.setAlpha(1.0f); On 2016/11/16 19:29:40, Ted C wrote: > why the need to set the alpha to 1 here? It was because I set the alpha initially to 0 and even with the setFillAfter(true) it did not stay visible. But I found a cleaner way. Thanks for the comment. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:293: mHandler.postDelayed(new Runnable() { On 2016/11/16 19:29:40, Ted C wrote: > If you were to add an AnimationListener to the Animation, would onAnimationEnd > be called when you are overwriting the animations from subsequent calls? > > https://developer.android.com/reference/android/view/animation/Animation.Anim... > > In particular, that would be nicer than posting tasks. If the listener doesn't > work, you could pull out a Runnable. > > private Runnable mFadeOutRunnable = new Runnable() {...}; > > Then you could do: > > mHandler.removeCallbacks(mFadeOutRunnable); > mHandler.postDelayed(mFadeOutRunnable, UPDATE_TEXT_VISIBILITY_DURATION_MS); > > Then you wouldn't need the pendingAnimationCount. > > The Handler also poses an issue as it will run after this class has gone away, > so you could be posting tasks that are run when the view is detached from the > view hierarchy, so it is another reason to use the listener if you can. But I would still have to post a task, no? Because I still have to wait for UPDATE_TEXT_VISIBILITY_DURATION_MS after the call to the Listener's onAnimationStart/End? I implemented your second suggestions in the meantime :) thanks! https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:426: public void addUpdateText(String text) { On 2016/11/16 19:29:40, Ted C wrote: > all the other methods that do the same seem to be private. any reason for this > to be public? The had to be public to be accessible in the base class because the child was nested. It is now private in the child itself. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:427: mUpdatedView = new TextView(getContext()); On 2016/11/16 19:29:40, Ted C wrote: > maybe add an assert that mUpdatedView == null before this? Done. https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:662: addUpdateText(updatedText); On 2016/11/16 19:57:28, dfalcantara (check my queue) wrote: > On 2016/11/16 19:29:40, Ted C wrote: > > dfalcantara@ can probably better speak to this than me, but it looks like this > > should be done in createMainSectionContent. > > > > Then showUpdateIfTextChanged would exist within this class. To me, it seems > > similar to ExtraTextsSection and it's exposed setExtraTexts stuff. > > Yeah, this belongs in createMainSectionContent with the other controls that are > created for this section. If you don't put it there, it gets increasingly > harder to determine the order that controls will be added to the layout. Done.
https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:293: mHandler.postDelayed(new Runnable() { On 2016/11/17 20:14:39, sebsg wrote: > On 2016/11/16 19:29:40, Ted C wrote: > > If you were to add an AnimationListener to the Animation, would onAnimationEnd > > be called when you are overwriting the animations from subsequent calls? > > > > > https://developer.android.com/reference/android/view/animation/Animation.Anim... > > > > In particular, that would be nicer than posting tasks. If the listener > doesn't > > work, you could pull out a Runnable. > > > > private Runnable mFadeOutRunnable = new Runnable() {...}; > > > > Then you could do: > > > > mHandler.removeCallbacks(mFadeOutRunnable); > > mHandler.postDelayed(mFadeOutRunnable, UPDATE_TEXT_VISIBILITY_DURATION_MS); > > > > Then you wouldn't need the pendingAnimationCount. > > > > The Handler also poses an issue as it will run after this class has gone away, > > so you could be posting tasks that are run when the view is detached from the > > view hierarchy, so it is another reason to use the listener if you can. > > But I would still have to post a task, no? Because I still have to wait for > UPDATE_TEXT_VISIBILITY_DURATION_MS after the call to the Listener's > onAnimationStart/End? > > I implemented your second suggestions in the meantime :) thanks! Ah yes, I didn't see the two times were the same. Indeed, you'll need a handler then. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:53: * . LEFT SUMMARY TEXT | OPTIONAL UPDATE TEXT | RIGHT SUMMARY TEXT | LOGO | ADD . these comments should be moved to LineItemBreakdownSection https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:129: static final int UPDATE_TEXT_ANIMATION_DURATION_MS = 500; these should be moved to LineItemBreakdownSection same for the handler. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:356: // The summary section displays up to three TextViews side by side. comment update no longer applicable. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:635: public void addUpdateText(LinearLayout mainSectionLayout) { private? https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:646: updatedLayoutParams.weight = 1; I don't think you need this. Since you are aligning the text to the end, you can leave the left text as the only one defining a weight. Then you'd need LayoutParams.WRAP_CONTENT instead of 0 the line above. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:659: mSummaryLayout = (LinearLayout) mainSectionLayout.getChildAt(1); instead of getting the child by id. Just expose a protected getSummaryLayout on the parent. Or, you could pass the summarylayout into createMainSectionContent. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:739: (TextView) mSummaryLayout.getChildAt(mSummaryLayout.getChildCount() - 1); I'd also expose a protected method, getRightSummary, or getRightSummaryText to avoid having to do this. Another thing that you could do is override setSummaryText and cache the previous text there. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:746: if (!TextUtils.equals(summaryRightTextView.getText().toString(), rightText.toString()) you shouldn't need the .toString()s for this method as it does CharSequence as input.
Thanks a bunch for the comments :) Another look? https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:53: * . LEFT SUMMARY TEXT | OPTIONAL UPDATE TEXT | RIGHT SUMMARY TEXT | LOGO | ADD . On 2016/11/17 23:30:55, Ted C wrote: > these comments should be moved to LineItemBreakdownSection Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:129: static final int UPDATE_TEXT_ANIMATION_DURATION_MS = 500; On 2016/11/17 23:30:55, Ted C wrote: > these should be moved to LineItemBreakdownSection > > same for the handler. Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:356: // The summary section displays up to three TextViews side by side. On 2016/11/17 23:30:55, Ted C wrote: > comment update no longer applicable. Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:635: public void addUpdateText(LinearLayout mainSectionLayout) { On 2016/11/17 23:30:55, Ted C wrote: > private? Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:646: updatedLayoutParams.weight = 1; On 2016/11/17 23:30:55, Ted C wrote: > I don't think you need this. Since you are aligning the text to the end, you > can leave the left text as the only one defining a weight. > > Then you'd need LayoutParams.WRAP_CONTENT instead of 0 the line above. Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:659: mSummaryLayout = (LinearLayout) mainSectionLayout.getChildAt(1); On 2016/11/17 23:30:55, Ted C wrote: > instead of getting the child by id. Just expose a protected getSummaryLayout on > the parent. > > Or, you could pass the summarylayout into createMainSectionContent. Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:739: (TextView) mSummaryLayout.getChildAt(mSummaryLayout.getChildCount() - 1); On 2016/11/17 23:30:55, Ted C wrote: > I'd also expose a protected method, getRightSummary, or getRightSummaryText to > avoid having to do this. > > Another thing that you could do is override setSummaryText and cache the > previous text there. Done. https://codereview.chromium.org/2503543003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:746: if (!TextUtils.equals(summaryRightTextView.getText().toString(), rightText.toString()) On 2016/11/17 23:30:55, Ted C wrote: > you shouldn't need the .toString()s for this method as it does CharSequence as > input. Done.
lgtm w/ a couple tiny nits https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:53: * . LEFT SUMMARY TEXT | | RIGHT SUMMARY TEXT | LOGO | ADD . looks like there is a unnecessary | left behind https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:61: * via need to fix the line wrapping/undo these changes altogether.
Thanks, sending to CQ https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:53: * . LEFT SUMMARY TEXT | | RIGHT SUMMARY TEXT | LOGO | ADD . On 2016/11/18 17:13:54, Ted C wrote: > looks like there is a unnecessary | left behind Done. https://codereview.chromium.org/2503543003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:61: * via On 2016/11/18 17:13:54, Ted C wrote: > need to fix the line wrapping/undo these changes altogether. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2503543003/#ps180001 (title: "Fixed Comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== [Payments] Add text when shopping card total is updated. BUG=662503 ========== to ========== [Payments] Add text when shopping card total is updated. BUG=662503 Committed: https://crrev.com/0d9dfefb340989c2cb8655bfbc3f6dd83d352c10 Cr-Commit-Position: refs/heads/master@{#433259} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0d9dfefb340989c2cb8655bfbc3f6dd83d352c10 Cr-Commit-Position: refs/heads/master@{#433259} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
