Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805273002:
[Payment Request] Selector view controller (Closed)
|
Created:
3 years, 8 months ago by Moe Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, mahmadi+paymentsioswatch_chromium.org, pkl (ping after 24h if needed), gogerald+paymentswatch_chromium.org, rouslan+payments_chromium.org, marq+watch_chromium.org, noyau+watch_chromium.org, mahmadi+paymentswatch_chromium.org, lpromero+watch_chromium.org, sebsg+paymentswatch_chromium.org, ios-reviews+showcase_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payment Request] Selector view controller
This view controller is going to replace the following view controllers:
PaymentMethodSelectionViewController
ShippingAddressSelectionViewController
ShippingOptionSelectionViewController
BUG=602666
http://imgur.com/a/nPHzd
Review-Url: https://codereview.chromium.org/2805273002
Cr-Commit-Position: refs/heads/master@{#465221}
Committed: https://chromium.googlesource.com/chromium/src/+/07c27c592ab8b9e59777084bc9e869dce6c5e23f
Patch Set 1 #
Total comments: 55
Patch Set 2 : Addressed comments #
Total comments: 17
Patch Set 3 : Addressed comments #
Total comments: 17
Patch Set 4 : Addressed comments #
Total comments: 22
Patch Set 5 : Addressed comments #
Dependent Patchsets: Messages
Total messages: 35 (18 generated)
The CQ bit was checked by mahmadi@chromium.org 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...
Description was changed from ========== [Payment Request] Selector view controller BUG=602666 http://imgur.com/a/nPHzd ========== to ========== [Payment Request] Selector view controller This view controller is going to replace the following view controllers: PaymentMethodSelectionViewController ShippingAddressSelectionViewController ShippingOptionSelectionViewController BUG=602666 http://imgur.com/a/nPHzd ==========
mahmadi@chromium.org changed reviewers: + gambard@chromium.org
Hi Gauthier, Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gambard@chromium.org changed reviewers: + lpromero@chromium.org
Adding lpromero for comments with +lpromero. This CL is big. Can you split it between the collection view part and the Showcase part? For now I only reviewed the view controller part. As I high level comment, I am not sure the data source should directly return the CollectionViewItem instead of returning the data necessary to build them in the collection. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:40: ItemTypeHeader = kItemTypeHeaderItem, // This is a repeated item type. Is this item type the same as the one in the data source? If it is the case I am not sure it is a good idea, the order would be very important. (If a new item type is added between header and selectable item, I guess everything would be broken.) +lpromero. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:66: self.navigationItem.leftBarButtonItem = returnButton; I think the coordinator should set the navigation item. I guess it would be OK if this method is used in tests only, but in this case make it more visible (put it in a testing category, name it initForTesting). https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:70: Add #pragma mark - PaymentRequestSelectorViewControllerActions https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:72: [_delegate paymentRequestSelectorViewControllerDidReturn:self]; s/_delegate/self.delegate in the whole file (except if the method can be called from init). https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:85: if (_dataSource.state == PaymentRequestSelectorViewControllerStatePending) { s/_dataSource/self.dataSource in the whole file (except if the method can be called from init). https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:87: statusItem.text = l10n_util::GetNSString(IDS_PAYMENTS_CHECKING_OPTION); Don't you need to set the status? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:92: CollectionViewItem* headerItem = [_dataSource headerItem]; I think it is better to pass the value to be displayed in the items than to pass directly the items. I understand this design is taking some logic out of the UI, allowing it to only display a bunch of item of any type. I personally have a UI mediator, translating the data send by the model to UI item, but it creates a lot of boilerplate. +lpromero for opinion. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:94: [model addItem:headerItem toSectionWithIdentifier:SectionIdentifierItems]; Use |setHeader:forSectionWithIdentifier:|? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:99: CollectionViewItem* item = [selectableItems objectAtIndex:index]; Nit: you can use: selectableItems[index] https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:100: DCHECK(item); You cannot have nil items in an array. You should remove this DCHECK. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:108: if ([[_dataSource addButtonTitle] length]) { Why are you returning items for all the others methods and a string for the button? +lpromero https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:131: - (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView I don't think this is how the items should be used. What do you think of modifying the PaymentsTextCell to add a text color property and removing the |collectionView:cellForItemAtIndexPath:|? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:144: if ([cell isKindOfClass:[PaymentsTextCell class]]) { I think this is a discrepancy between the idea you are trying to implement with passing directly the item to the view controller (i.e. in my understanding the View Controller is agnostic of the kind of item it receives), and this function where the ViewController needs to know exactly the class of the item it received. +lpromero https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:148: _dataSource.state == PaymentRequestSelectorViewControllerStateError Does the data source state is supposed to change over time? This means the state would be updated only when the cells are displayed the first time or when the cell is reused (not sure it is what you want). https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:181: DCHECK(selectedItem); Same, I don't think you need this DCHECK. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:191: [_dataSource setSelectedItemIndex:index]; I don't think the ViewController should set things directly in the data source. Can this be routed through the delegate? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:218: switch (item.type) { Nit: I would remove the the switch. Is there a particular reason for adding it? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:224: cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) I don't know if this is relevant in your case, but as you are using the card format, the width available for the cells is smaller than collectionView.bound. You can get the real width by doing: UIEdgeInsets inset = [self collectionView:collectionView layout:collectionView.collectionViewLayout insetForSectionAtIndex:indexPath.section]; return [MDCCollectionViewCell cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) - inset.left - inset.right forItem:item]; https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:235: if (type == ItemTypeHeader) { I would have say the ink does not appear on headers by default. Is it the case now? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:15: const NSInteger kItemTypeHeaderItem = kItemTypeEnumZero; We usually prefer the type to be defined only in the View Controller. +lpromero for opinion. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:18: // The possible states the view controller can be in. I am not sure those are the states of the view controller. They are the state of the selector, reflected by the view controller. What about PaymentRequestSelectorState? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:34: @property(nonatomic, assign) PaymentRequestSelectorViewControllerState state; I would mark them as readonly. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:20: @interface PaymentRequestSelectorMediator Do you really want to create a class with a name as generic as PaymentRequestSelectorMediator for one test? Can you prefix/suffix it with test? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:38: [[CollectionViewTextItem alloc] initWithType:kItemTypeSelectableItem], This would DCHECK in the styling done in cellForItemAtIndexPath: https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/ui/colle... File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/ui/colle... ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: @property(nonatomic) MDCCollectionViewCellAccessoryType accessoryType; I will let lpromero answer about this change, I am not sure we want to add functionalities to the CollectionViewItem. It could be solved if you don't pass CollectionViewItem in the Data Source :)
Thanks Gauthier. PTAL. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:40: ItemTypeHeader = kItemTypeHeaderItem, // This is a repeated item type. On 2017/04/10 09:47:35, gambard wrote: > Is this item type the same as the one in the data source? If it is the case I am > not sure it is a good idea, the order would be very important. (If a new item > type is added between header and selectable item, I guess everything would be > broken.) > +lpromero. Correct. Nothing should be added between the header and the selectable item. But since we're writing this once and reusing it, nothing should ideally be added here anyway. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:66: self.navigationItem.leftBarButtonItem = returnButton; On 2017/04/10 09:47:34, gambard wrote: > I think the coordinator should set the navigation item. I guess it would be OK > if this method is used in tests only, but in this case make it more visible (put > it in a testing category, name it initForTesting). All the view controllers in ios/chrome/browser/payments setup their own navigation item. I'd say let's keep this consistent with the rest of the view controllers and if we really need to this, we'll do it a sweeping stroke for all view controllers. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:70: On 2017/04/10 09:47:34, gambard wrote: > Add #pragma mark - PaymentRequestSelectorViewControllerActions Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:72: [_delegate paymentRequestSelectorViewControllerDidReturn:self]; On 2017/04/10 09:47:35, gambard wrote: > s/_delegate/self.delegate in the whole file (except if the method can be called > from init). Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:85: if (_dataSource.state == PaymentRequestSelectorViewControllerStatePending) { On 2017/04/10 09:47:35, gambard wrote: > s/_dataSource/self.dataSource in the whole file (except if the method can be > called from init). Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:87: statusItem.text = l10n_util::GetNSString(IDS_PAYMENTS_CHECKING_OPTION); On 2017/04/10 09:47:35, gambard wrote: > Don't you need to set the status? Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:92: CollectionViewItem* headerItem = [_dataSource headerItem]; On 2017/04/10 09:47:35, gambard wrote: > I think it is better to pass the value to be displayed in the items than to pass > directly the items. > I understand this design is taking some logic out of the UI, allowing it to only > display a bunch of item of any type. > I personally have a UI mediator, translating the data send by the model to UI > item, but it creates a lot of boilerplate. > +lpromero for opinion. I discussed this with lpromero@ previously and he supported the idea of passing the items directly. I personally find another abstraction level a bit of an overkill. But could you please provide an example of your UI mediator for context? https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:94: [model addItem:headerItem toSectionWithIdentifier:SectionIdentifierItems]; On 2017/04/10 09:47:35, gambard wrote: > Use |setHeader:forSectionWithIdentifier:|? Unfortunately we don't have support for arbitrary sized headers just yet: https://cs.chromium.org/webrtc/src/ios/chrome/browser/ui/collection_view/coll... https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:99: CollectionViewItem* item = [selectableItems objectAtIndex:index]; On 2017/04/10 09:47:34, gambard wrote: > Nit: you can use: selectableItems[index] Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:100: DCHECK(item); On 2017/04/10 09:47:34, gambard wrote: > You cannot have nil items in an array. You should remove this DCHECK. Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:108: if ([[_dataSource addButtonTitle] length]) { On 2017/04/10 09:47:35, gambard wrote: > Why are you returning items for all the others methods and a string for the > button? > +lpromero Because the add button, if any, will be the same kind and style for all instances of this view controller. why pushing the logic to the mediator when it can be created and configured by the view controller. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:131: - (UICollectionViewCell*)collectionView:(UICollectionView*)collectionView On 2017/04/10 09:47:34, gambard wrote: > I don't think this is how the items should be used. What do you think of > modifying the PaymentsTextCell to add a text color property and removing the > |collectionView:cellForItemAtIndexPath:|? I agree. Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:144: if ([cell isKindOfClass:[PaymentsTextCell class]]) { On 2017/04/10 09:47:35, gambard wrote: > I think this is a discrepancy between the idea you are trying to implement with > passing directly the item to the view controller (i.e. in my understanding the > View Controller is agnostic of the kind of item it receives), and this function > where the ViewController needs to know exactly the class of the item it > received. > +lpromero I see your point of view. Again this was another point I discussed with lpromero@ prior to implementing this. In fact what happening here isn't correct. the mediator who passes the item to the view controller should also be responsible for configuring the item and therefore the cell. Otherwise, the configured cell that's returned by this method can be overridden by "item.configureCell". If the item is configured in the mediator, the view controller can be "more or less" completely agnostic of the kind of items it displays. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:148: _dataSource.state == PaymentRequestSelectorViewControllerStateError On 2017/04/10 09:47:35, gambard wrote: > Does the data source state is supposed to change over time? > This means the state would be updated only when the cells are displayed the > first time or when the cell is reused (not sure it is what you want). because the state changes always involve going to/from the pending state, the coordinator asks view controller to reload the view. So this should work just fine. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:181: DCHECK(selectedItem); On 2017/04/10 09:47:35, gambard wrote: > Same, I don't think you need this DCHECK. Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:191: [_dataSource setSelectedItemIndex:index]; On 2017/04/10 09:47:34, gambard wrote: > I don't think the ViewController should set things directly in the data source. > Can this be routed through the delegate? Good point. Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:218: switch (item.type) { On 2017/04/10 09:47:35, gambard wrote: > Nit: I would remove the the switch. Is there a particular reason for adding it? Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:224: cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) On 2017/04/10 09:47:35, gambard wrote: > I don't know if this is relevant in your case, but as you are using the card > format, the width available for the cells is smaller than collectionView.bound. > You can get the real width by doing: > UIEdgeInsets inset = [self collectionView:collectionView > layout:collectionView.collectionViewLayout > insetForSectionAtIndex:indexPath.section]; > > return [MDCCollectionViewCell > cr_preferredHeightForWidth:CGRectGetWidth(collectionView.bounds) - > inset.left - inset.right > forItem:item]; Definitely relevant. Thank you. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:235: if (type == ItemTypeHeader) { On 2017/04/10 09:47:34, gambard wrote: > I would have say the ink does not appear on headers by default. Is it the case > now? please see above. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:18: // The possible states the view controller can be in. On 2017/04/10 09:47:35, gambard wrote: > I am not sure those are the states of the view controller. They are the state of > the selector, reflected by the view controller. What about > PaymentRequestSelectorState? Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:34: @property(nonatomic, assign) PaymentRequestSelectorViewControllerState state; On 2017/04/10 09:47:35, gambard wrote: > I would mark them as readonly. Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:20: @interface PaymentRequestSelectorMediator On 2017/04/10 09:47:35, gambard wrote: > Do you really want to create a class with a name as generic as > PaymentRequestSelectorMediator for one test? > Can you prefix/suffix it with test? Done. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:38: [[CollectionViewTextItem alloc] initWithType:kItemTypeSelectableItem], On 2017/04/10 09:47:35, gambard wrote: > This would DCHECK in the styling done in cellForItemAtIndexPath: It wouldn't because it has an expected item type.
Patchset #2 (id:20001) has been deleted
mainly lg. I will wait for lpromero's opinion for a more thoughtful review https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/cells/payments_text_item.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/cells/payments_text_item.mm:64: cell.textLabel.numberOfLines = 0; For me the item sets only the properties that might be different. I am find with having a default styling with cell.textLabel.numberOfLines = 0; cell.textLabel.lineBreakMode = NSLineBreakByWordWrapping; inside the cell instead of in the item.
First round before going in a meeting. I will finish the review today. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:140: case ItemTypeSelectableItem: { Move this right above the default case to bundle them in one break statement. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:144: if ([cell isKindOfClass:[PaymentsTextCell class]]) { On 2017/04/10 17:53:49, moe wrote: > On 2017/04/10 09:47:35, gambard wrote: > > I think this is a discrepancy between the idea you are trying to implement > with > > passing directly the item to the view controller (i.e. in my understanding the > > View Controller is agnostic of the kind of item it receives), and this > function > > where the ViewController needs to know exactly the class of the item it > > received. > > +lpromero > > I see your point of view. Again this was another point I discussed with > lpromero@ prior to implementing this. In fact what happening here isn't correct. > the mediator who passes the item to the view controller should also be > responsible for configuring the item and therefore the cell. Otherwise, the > configured cell that's returned by this method can be overridden by > "item.configureCell". If the item is configured in the mediator, the view > controller can be "more or less" completely agnostic of the kind of items it > displays. It's the idea of an agnostic view controller that Rohit wants to avoid, though. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/BUILD.gn:92: source_set("payments_ui") { Moe, can you take the action item to split payments/ into: i/c/b/payments for model stuff i/c/b/ui/payments:payments for coordinators stuff (mediators, etc.) i/c/b/ui/payments:payments_ui for this UI layer target? Because theoretically, UI shouldn't be in i/c/b/payments. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/cells/payments_text_item.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/cells/payments_text_item.h:23: @property(nonatomic, null_resettable, copy) UIFont* textFont; This goes on the "styling" idea territory. Is this just for the support of header vs normal cell? If so, I would create two items and two cells. Or just have an enum with the style. That way the item keeps being descriptive rather than imperative. The cell can then choose the font and color based on the style enum. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/cells/payments_text_item.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/cells/payments_text_item.mm:64: cell.textLabel.numberOfLines = 0; On 2017/04/11 15:23:25, gambard wrote: > For me the item sets only the properties that might be different. I am find with > having a default styling with cell.textLabel.numberOfLines = 0; > cell.textLabel.lineBreakMode = NSLineBreakByWordWrapping; inside the cell > instead of in the item. +1 https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h:13: - (void)onReturn; What is the return button in the screenshot? Maybe call it onSelection? https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: @property(nonatomic) MDCCollectionViewCellAccessoryType accessoryType; I proposed this change several times to Rohit :) Can you make it its own CL and send it to me and him? I would support this as it means CollectionViewItem matches 1:1 with an MDCCollectionViewCell, which is pretty bare, so it would not add more logic than what we already have for accessibility traits and accessibility identifier. Note that currently, several subclasses have this property, so there is now a conflict. You should remove it from all the subclasses.
https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:79: - (void)loadModel { I would have seen the model being entirely created by the mediator instead of mix and match. But as Gauthier raises the issues, this was probably a bad idea I had :( It makes more sense to have a simpler data source API that only asks for the relevant bits of info. Since this controller is pretty specific in its use case (list of strings, add button), I think it would be a better way to reuse code. Sorry for throwing you in that direction :( https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:145: CollectionViewItem* selectedItem = [self.dataSource Are all your items PaymentsTextItem? If so, it should be the one having the accessoryType property in case it is not approved to move in the parent class. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h:13: - (void)onReturn; On 2017/04/11 15:59:06, lpromero wrote: > What is the return button in the screenshot? Maybe call it onSelection? Got it. The return button is present in the normal use case, not Showcase. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:23: @property(nonatomic, strong) NSArray<CollectionViewItem*>* items; Convention is to use s/strong/copy as the type has a mutable variant (NSArray, NSDictionary, NSString, etc.).
Hi, PTAL https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:144: if ([cell isKindOfClass:[PaymentsTextCell class]]) { On 2017/04/11 15:59:06, lpromero wrote: > On 2017/04/10 17:53:49, moe wrote: > > On 2017/04/10 09:47:35, gambard wrote: > > > I think this is a discrepancy between the idea you are trying to implement > > with > > > passing directly the item to the view controller (i.e. in my understanding > the > > > View Controller is agnostic of the kind of item it receives), and this > > function > > > where the ViewController needs to know exactly the class of the item it > > > received. > > > +lpromero > > > > I see your point of view. Again this was another point I discussed with > > lpromero@ prior to implementing this. In fact what happening here isn't > correct. > > the mediator who passes the item to the view controller should also be > > responsible for configuring the item and therefore the cell. Otherwise, the > > configured cell that's returned by this method can be overridden by > > "item.configureCell". If the item is configured in the mediator, the view > > controller can be "more or less" completely agnostic of the kind of items it > > displays. > > It's the idea of an agnostic view controller that Rohit wants to avoid, though. Brought this back as we decided the mediators shouldn't be styling the cells. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/BUILD.gn:92: source_set("payments_ui") { On 2017/04/11 15:59:06, lpromero wrote: > Moe, can you take the action item to split payments/ into: > > i/c/b/payments for model stuff > i/c/b/ui/payments:payments for coordinators stuff (mediators, etc.) > i/c/b/ui/payments:payments_ui for this UI layer target? > > Because theoretically, UI shouldn't be in i/c/b/payments. sg. I have a bunch of UI CLs stacked up at the moment. Will do this in a separate CL as soon as they land. Otherwise it'll be a nightmare to rebase those. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/cells/payments_text_item.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/cells/payments_text_item.h:23: @property(nonatomic, null_resettable, copy) UIFont* textFont; On 2017/04/11 15:59:06, lpromero wrote: > This goes on the "styling" idea territory. Is this just for the support of > header vs normal cell? > If so, I would create two items and two cells. Or just have an enum with the > style. That way the item keeps being descriptive rather than imperative. The > cell can then choose the font and color based on the style enum. Removed the style properties from the item and moved the defaults back to the cell, having the CVC override them when necessary. similar to: https://codereview.chromium.org/2817953002/ https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/cells/payments_text_item.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/cells/payments_text_item.mm:64: cell.textLabel.numberOfLines = 0; On 2017/04/11 15:23:25, gambard wrote: > For me the item sets only the properties that might be different. I am find with > having a default styling with cell.textLabel.numberOfLines = 0; > cell.textLabel.lineBreakMode = NSLineBreakByWordWrapping; inside the cell > instead of in the item. Done. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:79: - (void)loadModel { On 2017/04/11 17:30:19, lpromero wrote: > I would have seen the model being entirely created by the mediator instead of > mix and match. But as Gauthier raises the issues, this was probably a bad idea I > had :( It makes more sense to have a simpler data source API that only asks for > the relevant bits of info. > > Since this controller is pretty specific in its use case (list of strings, add > button), I think it would be a better way to reuse code. > Sorry for throwing you in that direction :( Acknowledged. I think we're now on the same page about this model. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:145: CollectionViewItem* selectedItem = [self.dataSource On 2017/04/11 17:30:19, lpromero wrote: > Are all your items PaymentsTextItem? If so, it should be the one having the > accessoryType property in case it is not approved to move in the parent class. Unfortunately not. This also can't remain blocked on the accessory type property moving to the parent class. Can we use [(id)item setType:...] as a temporary measure until we resolve that issue? https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:23: @property(nonatomic, strong) NSArray<CollectionViewItem*>* items; On 2017/04/11 17:30:19, lpromero wrote: > Convention is to use s/strong/copy as the type has a mutable variant (NSArray, > NSDictionary, NSString, etc.). Done. https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/collection_view/cells/collection_view_item.h (right): https://codereview.chromium.org/2805273002/diff/40001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/collection_view/cells/collection_view_item.h:24: @property(nonatomic) MDCCollectionViewCellAccessoryType accessoryType; On 2017/04/11 15:59:06, lpromero wrote: > I proposed this change several times to Rohit :) > Can you make it its own CL and send it to me and him? > I would support this as it means CollectionViewItem matches 1:1 with an > MDCCollectionViewCell, which is pretty bare, so it would not add more logic than > what we already have for accessibility traits and accessibility identifier. > > Note that currently, several subclasses have this property, so there is now a > conflict. You should remove it from all the subclasses. Done.
Patchset #3 (id:60001) has been deleted
lg when we are through with the itemType API refactoring. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:40: ItemTypeHeader = kItemTypeHeaderItem, // This is a repeated item type. I think the comment applies to the following line. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:65: returnButton.accessibilityLabel = l10n_util::GetNSString(IDS_ACCNAME_BACK); FYI, ChromeIcon returns images and button items that already have an accessibility label (in this case, it's https://cs.chromium.org/chromium/src/ios/chrome/app/strings/ios_strings.grd?t...). You could then remove the line, but I can also understand wanting to explicitly set the label. In both case, thanks for thinking about accessibility! https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:90: statusItem.text = l10n_util::GetNSString(IDS_PAYMENTS_CHECKING_OPTION); Would it make sens for these last two to be set in the mediator, or even be the default value in the item/cell? https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:102: for (NSUInteger index = 0; index < selectableItems.count; ++index) { Optional fancy nit: you could use -enumerateObjectsUsingBlock:. It gives you both the index and the object at index. It is said to be faster than fast enumeration, although that might not be really important here. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:105: [(id)item setAccessoryType:(index == self.dataSource.selectedItemIndex) This makes the assumption that CollectionViewItem has the accessory type, which I am not sure will go through: https://codereview.chromium.org/2814793003/ You might need to have -selectableItems return an NSArray<CollectionViewItem*<Selectable>> or something, and have your items implement the protocol. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:111: if ([[self.dataSource addButtonTitle] length]) { As Gauthier pointed out, since the mediator already outputs items, this one might as well come from the mediator. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h (right): https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:15: const NSInteger kItemTypeHeaderItem = kItemTypeEnumZero; When your "-setType:" CL goes in, this can be removed. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:8: #import "base/mac/scoped_nsobject.h" No need in ARC. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:86: base::scoped_nsobject<TestPaymentRequestSelectorMediator> mediator_; No need for scoped_nsobject in ARC. TestPaymentRequestSelectorMediator* mediator_;
Patchset #4 (id:100001) has been deleted
https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:40: ItemTypeHeader = kItemTypeHeaderItem, // This is a repeated item type. On 2017/04/14 12:18:10, lpromero wrote: > I think the comment applies to the following line. Done. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:65: returnButton.accessibilityLabel = l10n_util::GetNSString(IDS_ACCNAME_BACK); On 2017/04/14 12:18:10, lpromero wrote: > FYI, ChromeIcon returns images and button items that already have an > accessibility label (in this case, it's > https://cs.chromium.org/chromium/src/ios/chrome/app/strings/ios_strings.grd?t...). > You could then remove the line, but I can also understand wanting to explicitly > set the label. > In both case, thanks for thinking about accessibility! Thanks for catching this. in a follow up CL, will remove these accessibility labels from all view controllers in one go. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:90: statusItem.text = l10n_util::GetNSString(IDS_PAYMENTS_CHECKING_OPTION); On 2017/04/14 12:18:10, lpromero wrote: > Would it make sens for these last two to be set in the mediator, or even be the > default value in the item/cell? StatusItemState::VERIFYING is the default value for state. But since the default value is not something like "Normal" and instead is "Verifying" which shows a spinner, I think having it explicitly set makes it easier for readers to understand what is going on. As for the text, I think if the item is "static" (it's created with pre-known static properties across all mediators) it makes sense for it to be created in the CVC. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:102: for (NSUInteger index = 0; index < selectableItems.count; ++index) { On 2017/04/14 12:18:10, lpromero wrote: > Optional fancy nit: you could use -enumerateObjectsUsingBlock:. It gives you > both the index and the object at index. It is said to be faster than fast > enumeration, although that might not be really important here. Good one! Done. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:105: [(id)item setAccessoryType:(index == self.dataSource.selectedItemIndex) On 2017/04/14 12:18:10, lpromero wrote: > This makes the assumption that CollectionViewItem has the accessory type, which > I am not sure will go through: > https://codereview.chromium.org/2814793003/ > > You might need to have -selectableItems return an > NSArray<CollectionViewItem*<Selectable>> or something, and have your items > implement the protocol. Done. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:111: if ([[self.dataSource addButtonTitle] length]) { On 2017/04/14 12:18:10, lpromero wrote: > As Gauthier pointed out, since the mediator already outputs items, this one > might as well come from the mediator. Done. In this case since the text of the item varies between the mediators i agree that it makes sense for the mediator to create the item. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm (right): https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:8: #import "base/mac/scoped_nsobject.h" On 2017/04/14 12:18:10, lpromero wrote: > No need in ARC. Done. https://codereview.chromium.org/2805273002/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_selector_view_controller_unittest.mm:86: base::scoped_nsobject<TestPaymentRequestSelectorMediator> mediator_; On 2017/04/14 12:18:10, lpromero wrote: > No need for scoped_nsobject in ARC. > > TestPaymentRequestSelectorMediator* mediator_; Done.
lg once the item type can be set in the View Controller. Thanks for your patience :) https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:66: self.navigationItem.leftBarButtonItem = returnButton; On 2017/04/10 17:53:49, moe wrote: > On 2017/04/10 09:47:34, gambard wrote: > > I think the coordinator should set the navigation item. I guess it would be OK > > if this method is used in tests only, but in this case make it more visible > (put > > it in a testing category, name it initForTesting). > > All the view controllers in ios/chrome/browser/payments setup their own > navigation item. I'd say let's keep this consistent with the rest of the view > controllers and if we really need to this, we'll do it a sweeping stroke for all > view controllers. Acknowledged. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:92: CollectionViewItem* headerItem = [_dataSource headerItem]; On 2017/04/10 17:53:49, moe wrote: > On 2017/04/10 09:47:35, gambard wrote: > > I think it is better to pass the value to be displayed in the items than to > pass > > directly the items. > > I understand this design is taking some logic out of the UI, allowing it to > only > > display a bunch of item of any type. > > I personally have a UI mediator, translating the data send by the model to UI > > item, but it creates a lot of boilerplate. > > +lpromero for opinion. > > I discussed this with lpromero@ previously and he supported the idea of passing > the items directly. I personally find another abstraction level a bit of an > overkill. But could you please provide an example of your UI mediator for > context? I am writing the ContentSuggestions classes. My mediator lives in ios/chrome/browser/content_suggestions/ https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:94: [model addItem:headerItem toSectionWithIdentifier:SectionIdentifierItems]; On 2017/04/10 17:53:49, moe wrote: > On 2017/04/10 09:47:35, gambard wrote: > > Use |setHeader:forSectionWithIdentifier:|? > > Unfortunately we don't have support for arbitrary sized headers just yet: > https://cs.chromium.org/webrtc/src/ios/chrome/browser/ui/collection_view/coll... Acknowledged. https://codereview.chromium.org/2805273002/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:148: _dataSource.state == PaymentRequestSelectorViewControllerStateError On 2017/04/10 17:53:49, moe wrote: > On 2017/04/10 09:47:35, gambard wrote: > > Does the data source state is supposed to change over time? > > This means the state would be updated only when the cells are displayed the > > first time or when the cell is reused (not sure it is what you want). > > because the state changes always involve going to/from the pending state, the > coordinator asks view controller to reload the view. So this should work just > fine. Acknowledged. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:25: - (void)paymentRequestSelectorViewControllerDidReturn: Nit: this name might be more explicit (first time I read it I thought the VC returned a value). Also, it is "Will" more than a "Did" no? Maybe paymentRequestSelectorViewControllerWillGoBack ? https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:29: - (void)paymentRequestSelectorViewControllerDidSelectAddItem: Nit: The methods name for the delegate are not easy to understand but as I don't have a better suggestions, I am OK :) https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: action:@selector(onReturn)]; I am not sure this works, but I guess you will see it when implementing the delegate. I tested it in showcase and it did not work (there are actually two back buttons and you can select the second one). Moving it to viewDidLoad make it work, I think you should do that. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:106: item.accessibilityTraits |= UIAccessibilityTraitButton; Why do you need to set accessibilityTraits? Doesn't the cell already have some useful traits?
lgtm https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:25: - (void)paymentRequestSelectorViewControllerDidReturn: On 2017/04/18 08:06:37, gambard wrote: > Nit: this name might be more explicit (first time I read it I thought the VC > returned a value). Also, it is "Will" more than a "Did" no? > Maybe paymentRequestSelectorViewControllerWillGoBack ? Usually such delegate calls use "Did". I think you could rename simply in DidFinish, which is very often used. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:29: - (void)paymentRequestSelectorViewControllerDidSelectAddItem: On 2017/04/18 08:06:37, gambard wrote: > Nit: The methods name for the delegate are not easy to understand but as I don't > have a better suggestions, I am OK :) Maybe just moving it before the previous method would be sufficient? https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:51: // CollectionViewControllerStyleAppBar and sets up the leading (return) button. with the CollectionViewControllerStyleAppBar *style* https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:51: // CollectionViewControllerStyleAppBar and sets up the leading (return) button. s/return/back If you rename the protocol method to didFinish, it might be better to remove mentions of return and change them into "back". https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: action:@selector(onReturn)]; On 2017/04/18 08:06:37, gambard wrote: > I am not sure this works, but I guess you will see it when implementing the > delegate. > I tested it in showcase and it did not work (there are actually two back buttons > and you can select the second one). Moving it to viewDidLoad make it work, I > think you should do that. What is "this"? Nil-target? It means that it sends it through the responder chain starting with the button itself. The view controller should be in the chain so I think it should work. In Showcase, I think Moe will instantiate it with initWithStyle to not have two bars, which will bypass this init and back button. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:106: item.accessibilityTraits |= UIAccessibilityTraitButton; On 2017/04/18 08:06:37, gambard wrote: > Why do you need to set accessibilityTraits? Doesn't the cell already have some > useful traits? Not the button one. It's the VC that decides if there is an action or not, in didSelectCell. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h:12: // Called when the user presses the return button. Proposed nit: s/return/back https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:45: // The add button item, if any. Optional nit: The "Add" button item, or something similar?
I missed https://codereview.chromium.org/2814273004 so lgtm :) https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: action:@selector(onReturn)]; On 2017/04/18 08:43:46, lpromero wrote: > On 2017/04/18 08:06:37, gambard wrote: > > I am not sure this works, but I guess you will see it when implementing the > > delegate. > > I tested it in showcase and it did not work (there are actually two back > buttons > > and you can select the second one). Moving it to viewDidLoad make it work, I > > think you should do that. > > What is "this"? Nil-target? It means that it sends it through the responder > chain starting with the button itself. The view controller should be in the > chain so I think it should work. > In Showcase, I think Moe will instantiate it with initWithStyle to not have two > bars, which will bypass this init and back button. "This" is "Adding an action to the button". I know that the styling will be changed in showcase, I am just saying that I checkouted the CL and tested it: the action is not trigger if the button is configured in init. It is triggered if it is done in viewDidLoad. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:106: item.accessibilityTraits |= UIAccessibilityTraitButton; On 2017/04/18 08:43:46, lpromero wrote: > On 2017/04/18 08:06:37, gambard wrote: > > Why do you need to set accessibilityTraits? Doesn't the cell already have some > > useful traits? > > Not the button one. It's the VC that decides if there is an action or not, in > didSelectCell. Acknowledged.
Thank you both for this! It was a tough one but I think we have a clear direction now :) https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:25: - (void)paymentRequestSelectorViewControllerDidReturn: On 2017/04/18 08:43:45, lpromero wrote: > On 2017/04/18 08:06:37, gambard wrote: > > Nit: this name might be more explicit (first time I read it I thought the VC > > returned a value). Also, it is "Will" more than a "Did" no? > > Maybe paymentRequestSelectorViewControllerWillGoBack ? > > Usually such delegate calls use "Did". I think you could rename simply in > DidFinish, which is very often used. Done. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:29: - (void)paymentRequestSelectorViewControllerDidSelectAddItem: On 2017/04/18 08:43:45, lpromero wrote: > On 2017/04/18 08:06:37, gambard wrote: > > Nit: The methods name for the delegate are not easy to understand but as I > don't > > have a better suggestions, I am OK :) > > Maybe just moving it before the previous method would be sufficient? Done. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:51: // CollectionViewControllerStyleAppBar and sets up the leading (return) button. On 2017/04/18 08:43:45, lpromero wrote: > s/return/back > > If you rename the protocol method to didFinish, it might be better to remove > mentions of return and change them into "back". Done. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.h:51: // CollectionViewControllerStyleAppBar and sets up the leading (return) button. On 2017/04/18 08:43:45, lpromero wrote: > with the CollectionViewControllerStyleAppBar *style* Done. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: action:@selector(onReturn)]; On 2017/04/18 08:52:45, gambard wrote: > On 2017/04/18 08:43:46, lpromero wrote: > > On 2017/04/18 08:06:37, gambard wrote: > > > I am not sure this works, but I guess you will see it when implementing the > > > delegate. > > > I tested it in showcase and it did not work (there are actually two back > > buttons > > > and you can select the second one). Moving it to viewDidLoad make it work, I > > > think you should do that. > > > > What is "this"? Nil-target? It means that it sends it through the responder > > chain starting with the button itself. The view controller should be in the > > chain so I think it should work. > > In Showcase, I think Moe will instantiate it with initWithStyle to not have > two > > bars, which will bypass this init and back button. > > "This" is "Adding an action to the button". I know that the styling will be > changed in showcase, I am just saying that I checkouted the CL and tested it: > the action is not trigger if the button is configured in init. It is triggered > if it is done in viewDidLoad. Like Louis mentioned I initialize the CVC in the showcase to not have an app bar. Even if we make the button work once the action is setup in -viewDidLoad there will still be two bars I think and therefore two sets of buttons which cause issues with the EG selectors when testing. In my experience we're better of without the app bar. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller_actions.h:12: // Called when the user presses the return button. On 2017/04/18 08:43:46, lpromero wrote: > Proposed nit: s/return/back Done. https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller_data_source.h:45: // The add button item, if any. On 2017/04/18 08:43:46, lpromero wrote: > Optional nit: The "Add" button item, or something similar? Done.
The CQ bit was checked by mahmadi@chromium.org 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...
https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: action:@selector(onReturn)]; On 2017/04/18 12:43:30, moe wrote: > On 2017/04/18 08:52:45, gambard wrote: > > On 2017/04/18 08:43:46, lpromero wrote: > > > On 2017/04/18 08:06:37, gambard wrote: > > > > I am not sure this works, but I guess you will see it when implementing > the > > > > delegate. > > > > I tested it in showcase and it did not work (there are actually two back > > > buttons > > > > and you can select the second one). Moving it to viewDidLoad make it work, > I > > > > think you should do that. > > > > > > What is "this"? Nil-target? It means that it sends it through the responder > > > chain starting with the button itself. The view controller should be in the > > > chain so I think it should work. > > > In Showcase, I think Moe will instantiate it with initWithStyle to not have > > two > > > bars, which will bypass this init and back button. > > > > "This" is "Adding an action to the button". I know that the styling will be > > changed in showcase, I am just saying that I checkouted the CL and tested it: > > the action is not trigger if the button is configured in init. It is triggered > > if it is done in viewDidLoad. > > Like Louis mentioned I initialize the CVC in the showcase to not have an app > bar. Even if we make the button > work once the action is setup in -viewDidLoad there will still be two bars I > think and therefore two sets of buttons > which cause issues with the EG selectors when testing. In my experience we're > better of without the app bar. The "two buttons" things was just a repro step :) I should not have mentioned it as it is clearly confusing and I apologize. My point is: this does not seem to work. You should move all your code below "// Set up leading (return) button." to -viewDidLoad. I am fine with committing it as is, but keep it in mind when creating your delegate if this is not called.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/18 12:47:48, gambard wrote: > https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... > File ios/chrome/browser/payments/payment_request_selector_view_controller.mm > (right): > > https://codereview.chromium.org/2805273002/diff/120001/ios/chrome/browser/pay... > ios/chrome/browser/payments/payment_request_selector_view_controller.mm:64: > action:@selector(onReturn)]; > On 2017/04/18 12:43:30, moe wrote: > > On 2017/04/18 08:52:45, gambard wrote: > > > On 2017/04/18 08:43:46, lpromero wrote: > > > > On 2017/04/18 08:06:37, gambard wrote: > > > > > I am not sure this works, but I guess you will see it when implementing > > the > > > > > delegate. > > > > > I tested it in showcase and it did not work (there are actually two back > > > > buttons > > > > > and you can select the second one). Moving it to viewDidLoad make it > work, > > I > > > > > think you should do that. > > > > > > > > What is "this"? Nil-target? It means that it sends it through the > responder > > > > chain starting with the button itself. The view controller should be in > the > > > > chain so I think it should work. > > > > In Showcase, I think Moe will instantiate it with initWithStyle to not > have > > > two > > > > bars, which will bypass this init and back button. > > > > > > "This" is "Adding an action to the button". I know that the styling will be > > > changed in showcase, I am just saying that I checkouted the CL and tested > it: > > > the action is not trigger if the button is configured in init. It is > triggered > > > if it is done in viewDidLoad. > > > > Like Louis mentioned I initialize the CVC in the showcase to not have an app > > bar. Even if we make the button > > work once the action is setup in -viewDidLoad there will still be two bars I > > think and therefore two sets of buttons > > which cause issues with the EG selectors when testing. In my experience we're > > better of without the app bar. > > The "two buttons" things was just a repro step :) I should not have mentioned it > as it is clearly confusing and I apologize. > My point is: this does not seem to work. You should move all your code below "// > Set up leading (return) button." to -viewDidLoad. I am fine with committing it > as is, but keep it in mind when creating your delegate if this is not called. Will keep that in mind. Thank you.
The CQ bit was checked by mahmadi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org, lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2805273002/#ps140001 (title: "Addressed comments")
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": 140001, "attempt_start_ts": 1492523557103690,
"parent_rev": "13bdf6f4f92e80ed3b23ad8f2418a5d180e732dc", "commit_rev":
"07c27c592ab8b9e59777084bc9e869dce6c5e23f"}
Message was sent while issue was closed.
Description was changed from ========== [Payment Request] Selector view controller This view controller is going to replace the following view controllers: PaymentMethodSelectionViewController ShippingAddressSelectionViewController ShippingOptionSelectionViewController BUG=602666 http://imgur.com/a/nPHzd ========== to ========== [Payment Request] Selector view controller This view controller is going to replace the following view controllers: PaymentMethodSelectionViewController ShippingAddressSelectionViewController ShippingOptionSelectionViewController BUG=602666 http://imgur.com/a/nPHzd Review-Url: https://codereview.chromium.org/2805273002 Cr-Commit-Position: refs/heads/master@{#465221} Committed: https://chromium.googlesource.com/chromium/src/+/07c27c592ab8b9e59777084bc9e8... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/07c27c592ab8b9e59777084bc9e8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
