[Payment Request] Accepted credit card type icons in the credit card editor

[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666
http://imgur.com/a/CCs4H

Review-Url: https://codereview.chromium.org/2825143002
Cr-Commit-Position: refs/heads/master@{#466371}
Committed: https://chromium.googlesource.com/chromium/src/+/a7b608ab885ee4a75ae15c56e944aa052402cbd4

[Moe, 2017-04-18 20:33:42]

Description was changed from

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666
==========

to

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/AdIzG
==========

[Moe, 2017-04-19 23:13:38]

Patchset #1 (id:1) has been deleted

[Moe, 2017-04-19 23:15:43]

Description was changed from

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/AdIzG
==========

to

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/AdIzG
http://imgur.com/a/qZzZC
==========

[Moe, 2017-04-19 23:15:44]

mahmadi@chromium.org changed reviewers:
+ lpromero@chromium.org

[Moe, 2017-04-19 23:15:58]

Hi Louis, Please review this CL.

[lpromero, 2017-04-20 16:30:49]

Doesn't the insets look weird on the real screen?

Note that if you know the cell won't have a card style, you should feel free to
create your own section in the catalog without the card style. Whatever suits
you best.

[lpromero, 2017-04-20 16:59:52]

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm (right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:61:
UIStackView* _stackView;
How does the stack view behave if there are more elements than the screen can
fit?

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:133:
[_stackView addArrangedSubview:methodTypeIconView];
This might be very costly to add views here. I think it would be best to add
them in the methodTypeIconViews's setter.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:155:
[methodTypeIconView removeFromSuperview];
If you override the setter, you could do this there, and here you'd only call
self.methodTypeIconViews = nil;.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:162: return
[NSString stringWithFormat:@"%@", self.messageLabel.text];
This is insufficient for accessibility. To fix, you should set an accessibility
label on the images (UIImages passed to the item). I think the UIImage's
accessibility label will be forwarded to the UIImageView that displays them
automatically. So you might be able to create a concatenated string with
self.messageLabel.text and all methodTypeIconView.accessibilityLabel.

(You might use componentsJoinedByString to deal with the commas between the
cards.)

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm
(right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm:19:
TEST(AccepedPaymentMethodsItemTest, TextLabelsAndImages) {
*Accepted

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm:21:
[[AcceptedPaymentMethodsItem alloc] initWithType:0];
Nit: alloc] init] :p

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/credit_card_edit_view_controller.h (right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/credit_card_edit_view_controller.h:21: @protocol
CreditCardEditViewControllerDataSource<
It might be time to move this into its own _data_source.h file, in a follow-up
CL.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/credit_card_edit_view_controller.h:29: // accepted
payment methods. Returns nil if a server card is being edited.
The notion of server card seems to me more like a detail from your implementor
instead of this data source protocol, no? This could also apply to the other
comments. I am fine as is.

[Moe, 2017-04-20 20:23:39]

Description was changed from

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/AdIzG
http://imgur.com/a/qZzZC
==========

to

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/CCs4H
==========

[Moe, 2017-04-21 14:14:14]

Hi Louis, PTAL.

The insets are according to the mocks. But for the sake of utilizing the space
for the icons on smaller screens I removed the cell's internal padding and kept
the card style. (screenshots are updated).

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm (right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:61:
UIStackView* _stackView;
On 2017/04/20 16:59:51, lpromero wrote:
> How does the stack view behave if there are more elements than the screen can
> fit?

It would squeeze the ones that don't fit. I adjusted the sizes and paddings so
that maximum number of supported cards (i.e., 8) would fit on the smallest
supported screen size (i.e., iPhone 5)
That's what's done on other platform too. It's just not worth the effort to have
multiple rows of icons.
http://imgur.com/a/QqLLW

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:133:
[_stackView addArrangedSubview:methodTypeIconView];
On 2017/04/20 16:59:51, lpromero wrote:
> This might be very costly to add views here. I think it would be best to add
> them in the methodTypeIconViews's setter.

Done.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:155:
[methodTypeIconView removeFromSuperview];
On 2017/04/20 16:59:51, lpromero wrote:
> If you override the setter, you could do this there, and here you'd only call
> self.methodTypeIconViews = nil;.

Done.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:162: return
[NSString stringWithFormat:@"%@", self.messageLabel.text];
On 2017/04/20 16:59:51, lpromero wrote:
> This is insufficient for accessibility. To fix, you should set an
accessibility
> label on the images (UIImages passed to the item). I think the UIImage's
> accessibility label will be forwarded to the UIImageView that displays them
> automatically. So you might be able to create a concatenated string with
> self.messageLabel.text and all methodTypeIconView.accessibilityLabel.
> 
> (You might use componentsJoinedByString to deal with the commas between the
> cards.)

Done.
PS. UIImage's accessibility label doesn't seem to get forwarded to the
UIImageView.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm
(right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm:19:
TEST(AccepedPaymentMethodsItemTest, TextLabelsAndImages) {
On 2017/04/20 16:59:52, lpromero wrote:
> *Accepted

Done.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item_unittest.mm:21:
[[AcceptedPaymentMethodsItem alloc] initWithType:0];
On 2017/04/20 16:59:52, lpromero wrote:
> Nit: alloc] init] :p

good riddance! :D as you can see this CL has been sitting around for a while.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/credit_card_edit_view_controller.h (right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/credit_card_edit_view_controller.h:21: @protocol
CreditCardEditViewControllerDataSource<
On 2017/04/20 16:59:52, lpromero wrote:
> It might be time to move this into its own _data_source.h file, in a follow-up
> CL.

Acknowledged.

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/credit_card_edit_view_controller.h:29: // accepted
payment methods. Returns nil if a server card is being edited.
On 2017/04/20 16:59:52, lpromero wrote:
> The notion of server card seems to me more like a detail from your implementor
> instead of this data source protocol, no? This could also apply to the other
> comments. I am fine as is.

Agreed. Done.

[lpromero, 2017-04-21 14:49:41]

LGTM! Thank you for you patience with all the preliminary work that was needed!

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
File ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm (right):

https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:162: return
[NSString stringWithFormat:@"%@", self.messageLabel.text];
On 2017/04/21 14:14:14, moe wrote:
> On 2017/04/20 16:59:51, lpromero wrote:
> > This is insufficient for accessibility. To fix, you should set an
> accessibility
> > label on the images (UIImages passed to the item). I think the UIImage's
> > accessibility label will be forwarded to the UIImageView that displays them
> > automatically. So you might be able to create a concatenated string with
> > self.messageLabel.text and all methodTypeIconView.accessibilityLabel.
> > 
> > (You might use componentsJoinedByString to deal with the commas between the
> > cards.)
> 
> Done.
> PS. UIImage's accessibility label doesn't seem to get forwarded to the
> UIImageView.

Good to know. Thank you for the warning.

[Moe, 2017-04-21 15:53:03]

On 2017/04/21 14:49:41, lpromero wrote:
> LGTM! Thank you for you patience with all the preliminary work that was
needed!
> 
>
https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
> File ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm
(right):
> 
>
https://codereview.chromium.org/2825143002/diff/20001/ios/chrome/browser/paym...
> ios/chrome/browser/payments/cells/accepted_payment_methods_item.mm:162: return
> [NSString stringWithFormat:@"%@", self.messageLabel.text];
> On 2017/04/21 14:14:14, moe wrote:
> > On 2017/04/20 16:59:51, lpromero wrote:
> > > This is insufficient for accessibility. To fix, you should set an
> > accessibility
> > > label on the images (UIImages passed to the item). I think the UIImage's
> > > accessibility label will be forwarded to the UIImageView that displays
them
> > > automatically. So you might be able to create a concatenated string with
> > > self.messageLabel.text and all methodTypeIconView.accessibilityLabel.
> > > 
> > > (You might use componentsJoinedByString to deal with the commas between
the
> > > cards.)
> > 
> > Done.
> > PS. UIImage's accessibility label doesn't seem to get forwarded to the
> > UIImageView.
> 
> Good to know. Thank you for the warning.

Thank you for the great feedback and reviews!

[Moe, 2017-04-21 15:53:06]

The CQ bit was checked by mahmadi@chromium.org

[commit-bot: I haz the power, 2017-04-21 15:53:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 17:07:37]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492789986582440,
"parent_rev": "0452bcf300c90ef3707f530cdec61c5397627cc9", "commit_rev":
"a7b608ab885ee4a75ae15c56e944aa052402cbd4"}

[commit-bot: I haz the power, 2017-04-21 17:07:43]

Description was changed from

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/CCs4H
==========

to

==========
[Payment Request] Accepted credit card type icons in the credit card editor

BUG=602666

http://imgur.com/a/CCs4H

Review-Url: https://codereview.chromium.org/2825143002
Cr-Commit-Position: refs/heads/master@{#466371}
Committed:
https://chromium.googlesource.com/chromium/src/+/a7b608ab885ee4a75ae15c56e944...
==========

[commit-bot: I haz the power, 2017-04-21 17:07:45]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/a7b608ab885ee4a75ae15c56e944...