Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2744823003:
[Payment Request] Generic edit form (Closed)
|
Created:
3 years, 9 months ago by Moe Modified:
3 years, 9 months ago Reviewers:
lpromero 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, noyau+watch_chromium.org, marq+watch_chromium.org, mahmadi+paymentswatch_chromium.org, srahim+watch_chromium.org, sebsg+paymentswatch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payment Request] Generic edit form
http://imgur.com/a/KlNeX
BUG=602666
TBR=caitkp@
Review-Url: https://codereview.chromium.org/2744823003
Cr-Commit-Position: refs/heads/master@{#459111}
Committed: https://chromium.googlesource.com/chromium/src/+/33100857169da33f18778cb1f3073e607ee48e27
Patch Set 1 #
Total comments: 13
Patch Set 2 : Generic edit form + showCase #
Total comments: 40
Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : Addressed comments #
Total comments: 31
Patch Set 5 : Addressed comments #
Total comments: 8
Patch Set 6 : Addressed comments #Patch Set 7 : rebase #Messages
Total messages: 33 (17 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [Payment Request] Credit Card edit form BUG=602666 ========== to ========== [Payment Request] Credit Card edit form http://imgur.com/a/Qodac BUG=602666 ==========
[email protected] changed reviewers: + [email protected], [email protected]
Hi Louis, Please take a look at this CL. Mathieu, Please take a look at this autofill::-wise, especially in credit_card_edit_view_controller.mm PS. Sorry for the huge CL. It was difficult to have a working editor with less code.
On 2017/03/13 00:19:33, moe wrote: > Hi Louis, Please take a look at this CL. > > Mathieu, Please take a look at this autofill::-wise, especially in > credit_card_edit_view_controller.mm > > PS. Sorry for the huge CL. It was difficult to have a working editor with less > code. Stupid
Hi moe, indeed, it is pretty long to review. Can I advise to land the UI layer first, not connected to Chrome, but connected to Showcase? I can advise on how to integrate, but a simple use case can be found here: https://codereview.chromium.org/2712743005/ The idea is similar to the material_cell_catalog, but as a standalone app. The idea is to get quick access to any part of the UI, without any connection to a real backend. That way, you can also land things piecemeal, one screen at a time. Then at the end connect all the dots within Chrome. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/BUILD.gn:18: "credit_card_edit_view_controller.h", For a follow-up CL: could you move the UI pieces to a payments_ui target and integrate the view controller within Showcase? https://chromium.googlesource.com/chromium/src/+/master/ios/showcase/README.md https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/BUILD.gn:82: "//ios/chrome/browser/ui/settings/cells", How come payments code needs this? I think we should extract what is needed and move it to a shared location. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... File ios/chrome/browser/ui/settings/autofill_credit_card_edit_collection_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... ios/chrome/browser/ui/settings/autofill_credit_card_edit_collection_view_controller.mm:23: #import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h" Is this used here?
https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/credit_card_edit_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/credit_card_edit_coordinator.mm:39: [self.baseViewController.navigationController popViewControllerAnimated:YES]; Could you instead do: [_viewController.navigationController popViewControllerAnimated:YES]; I think it will alleviate the double stop issue and remove the dependency on the other CL. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_util.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_util.mm:53: NSString* GetBillingAddressLabelFromAutofillProfile( This could go in its own CL maybe? Also, could you add a test file and a test for this function? (It's ok leaving the other untested for now.) https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... File ios/chrome/browser/ui/settings/cells/autofill_edit_item.h (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... ios/chrome/browser/ui/settings/cells/autofill_edit_item.h:6: #define IOS_CHROME_BROWSER_UI_SETTINGS_CELLS_AUTOFILL_EDIT_ITEM_H_ This file should probably be repurposed for more generic use and be moved out of settings.
Description was changed from ========== [Payment Request] Credit Card edit form http://imgur.com/a/Qodac BUG=602666 ========== to ========== [Payment Request] Generic edit form BUG=602666 ==========
[email protected] changed reviewers: - [email protected]
Description was changed from ========== [Payment Request] Generic edit form BUG=602666 ========== to ========== [Payment Request] Generic edit form http://imgur.com/a/hAsJ7 BUG=602666 ==========
Hi Louis, Please take another look. The CL now only includes the generic edit form with integration with showCase. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/BUILD.gn:18: "credit_card_edit_view_controller.h", On 2017/03/13 16:42:36, lpromero wrote: > For a follow-up CL: could you move the UI pieces to a payments_ui target and > integrate the view controller within Showcase? > https://chromium.googlesource.com/chromium/src/+/master/ios/showcase/README.md Done. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/BUILD.gn:82: "//ios/chrome/browser/ui/settings/cells", On 2017/03/13 16:42:36, lpromero wrote: > How come payments code needs this? I think we should extract what is needed and > move it to a shared location. these are for the autofill_edit_accessory_view.h and autofill_edit_item.h. I agree. I'll add a task to do that in a follow up CL. how does ios/chrome/browser/ui/cells/ sound as the new home? https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/credit_card_edit_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/credit_card_edit_coordinator.mm:39: [self.baseViewController.navigationController popViewControllerAnimated:YES]; On 2017/03/13 16:54:09, lpromero wrote: > Could you instead do: > [_viewController.navigationController popViewControllerAnimated:YES]; > > I think it will alleviate the double stop issue and remove the dependency on the > other CL. It does! Thanks. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/payment_request_util.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/payment_request_util.mm:53: NSString* GetBillingAddressLabelFromAutofillProfile( On 2017/03/13 16:54:09, lpromero wrote: > This could go in its own CL maybe? Also, could you add a test file and a test > for this function? (It's ok leaving the other untested for now.) Done. Moved to its own CL. https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... File ios/chrome/browser/ui/settings/autofill_credit_card_edit_collection_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... ios/chrome/browser/ui/settings/autofill_credit_card_edit_collection_view_controller.mm:23: #import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h" On 2017/03/13 16:42:36, lpromero wrote: > Is this used here? Hmm. this looks wrong. I must've made a mistake trying to add this to ios/chrome/browser/payments/credit_card_edit_view_controller.mm https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... File ios/chrome/browser/ui/settings/cells/autofill_edit_item.h (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/ui/setti... ios/chrome/browser/ui/settings/cells/autofill_edit_item.h:6: #define IOS_CHROME_BROWSER_UI_SETTINGS_CELLS_AUTOFILL_EDIT_ITEM_H_ On 2017/03/13 16:54:09, lpromero wrote: > This file should probably be repurposed for more generic use and be moved out of > settings. I agree. This and autofill_edit_accessory_view.h.
https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/1/ios/chrome/browser/payments... ios/chrome/browser/payments/BUILD.gn:82: "//ios/chrome/browser/ui/settings/cells", On 2017/03/16 01:35:32, moe wrote: > On 2017/03/13 16:42:36, lpromero wrote: > > How come payments code needs this? I think we should extract what is needed > and > > move it to a shared location. > > these are for the autofill_edit_accessory_view.h and autofill_edit_item.h. > I agree. I'll add a task to do that in a follow up CL. how does > ios/chrome/browser/ui/cells/ sound as the new home? Would they be generic enough to be in ios/chrome/browser/ui/collection_view/cells? I think it would be OK. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:17: const NSInteger ItemTypeErrorMessage = kItemTypeEnumZero + 100; Did you mean to add this to the implementation file? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:19: // Field definition for an editor field. Used for building the UI and s/an/a https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:46: // Features sections for every EditorField returned by -editorFields. Each Nit: s/Features/features https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:51: // Subclasses override this method to returns a list of field definitions for s/returns/return https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:51: // Subclasses override this method to returns a list of field definitions for Is there an alternative to subclassing this controller? Can't it accept an editorFields vector as property? Will subclasses also implement custom behavior? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:55: // |autofillType|. |required| indicates wether this is a required field or not. s/wether/whether. I just found out "wether" exists too! https://www.google.fr/search?q=wether https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:56: // If there are no validation errors an empty string is returned. Comma after "errors". https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:75: // Enables or Disables the accessory view's previous and next buttons depending s/Disables/disables https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:76: // on wehther there is a text field before and after the currently focused text whether https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:78: - (void)updateAccessoryViewButtonState; There should be a plural somewhere. updateAccessoryViewButtonsStates? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:121: NSString* labelFormat = field.required ? @"%@*" : @"%@"; Since AutofillEditItem knows about the "required" concept (it has a "required" property), I think this should be handled by the associated cell. You can add an example to material_cell_catalog to show a "required" item for example. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:202: if ([cell isMemberOfClass:[AutofillEditCell class]]) { Optional nit: is isKindOfClass more appropriate? We shouldn’t ever have subclasses of AutofillEditCell, so we might not be hit, but conceptually, it seems you want to use isKindOfClass. -isKindOfClass: is a "class" or subclass of "class" -isMemberOfClass: is a "class" https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:238: NOTREACHED(); How come we don't reach this section? The text fields items are not of ItemTypeErrorMessage. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:260: - (NSIndexPath*)indexPathForCurrentTextField { This one is not protected, it is private. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:299: [self.collectionView reloadSections:[NSIndexSet indexSetWithIndex:section]]; Does that mean that the error message doesn't animate in? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:322: if ([nextCell isMemberOfClass:[AutofillEditCell class]]) { Idem, would isKindOfClass be better? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:340: - (void)keyboardDidShow { Weird, I thought UICollectionViewController already did that out of the box. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:341: [self.collectionView scrollRectToVisible:[_currentEditingCell frame] No action needed: I wonder if using [self.collectionView scrollToItemAtIndexPath:[self.collectionView indexPathForCell:_currentEditingCell] atScrollPosition:UICollectionViewScrollPositionCenteredVertically animated:YES] would be less brittle. https://codereview.chromium.org/2744823003/diff/60001/ios/showcase/payments/s... File ios/showcase/payments/sc_payments_editor_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/showcase/payments/s... ios/showcase/payments/sc_payments_editor_coordinator.mm:22: SectionIdentifierAddress, How will you use PaymentRequestEditViewController in Chrome? Subclasses for different screens?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:17: const NSInteger ItemTypeErrorMessage = kItemTypeEnumZero + 100; On 2017/03/16 13:01:06, lpromero wrote: > Did you mean to add this to the implementation file? Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:19: // Field definition for an editor field. Used for building the UI and On 2017/03/16 13:01:06, lpromero wrote: > s/an/a It should be "an editor...", right? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:46: // Features sections for every EditorField returned by -editorFields. Each On 2017/03/16 13:01:06, lpromero wrote: > Nit: s/Features/features Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:51: // Subclasses override this method to returns a list of field definitions for On 2017/03/16 13:01:06, lpromero wrote: > Is there an alternative to subclassing this controller? Can't it accept an > editorFields vector as property? Will subclasses also implement custom behavior? I can definitely pass those as a property, but unfortunately it's not just the field definitions that vary. The credit card editor and the address/contact info editors feature some extra UI elements and functionality and slightly different validation/submission behaviors. It's hard to know how much they can be refactored before having both editors. But I'm more than happy to continue refactoring the common UI and functionality into the generic editor and keep the subclasses as shallow as possible. go/pr-bling-mocks https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:51: // Subclasses override this method to returns a list of field definitions for On 2017/03/16 13:01:06, lpromero wrote: > s/returns/return Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:55: // |autofillType|. |required| indicates wether this is a required field or not. On 2017/03/16 13:01:07, lpromero wrote: > s/wether/whether. I just found out "wether" exists too! > https://www.google.fr/search?q=wether Done! definitely whether and not wether here :) https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:56: // If there are no validation errors an empty string is returned. On 2017/03/16 13:01:07, lpromero wrote: > Comma after "errors". Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:75: // Enables or Disables the accessory view's previous and next buttons depending On 2017/03/16 13:01:06, lpromero wrote: > s/Disables/disables Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:76: // on wehther there is a text field before and after the currently focused text On 2017/03/16 13:01:06, lpromero wrote: > whether Done. Sorry. dyslexia... https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:78: - (void)updateAccessoryViewButtonState; On 2017/03/16 13:01:06, lpromero wrote: > There should be a plural somewhere. updateAccessoryViewButtonsStates? Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:121: NSString* labelFormat = field.required ? @"%@*" : @"%@"; On 2017/03/16 13:01:07, lpromero wrote: > Since AutofillEditItem knows about the "required" concept (it has a "required" > property), I think this should be handled by the associated cell. You can add an > example to material_cell_catalog to show a "required" item for example. Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:202: if ([cell isMemberOfClass:[AutofillEditCell class]]) { On 2017/03/16 13:01:07, lpromero wrote: > Optional nit: is isKindOfClass more appropriate? We shouldn’t ever have > subclasses of AutofillEditCell, so we might not be hit, but conceptually, it > seems you want to use isKindOfClass. > > -isKindOfClass: is a "class" or subclass of "class" > -isMemberOfClass: is a "class" Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:238: NOTREACHED(); On 2017/03/16 13:01:06, lpromero wrote: > How come we don't reach this section? The text fields items are not of > ItemTypeErrorMessage. That's the subclasses who decide what the item types for the fields (and other items they might add to the model) are and they should handle them before calling the super method. But I think it's worth adding a condition here to check if the item is a kind of AutofillEditItem so that simple subclasses don't need to override this method. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:260: - (NSIndexPath*)indexPathForCurrentTextField { On 2017/03/16 13:01:07, lpromero wrote: > This one is not protected, it is private. Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:299: [self.collectionView reloadSections:[NSIndexSet indexSetWithIndex:section]]; On 2017/03/16 13:01:07, lpromero wrote: > Does that mean that the error message doesn't animate in? Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:322: if ([nextCell isMemberOfClass:[AutofillEditCell class]]) { On 2017/03/16 13:01:06, lpromero wrote: > Idem, would isKindOfClass be better? Done. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:341: [self.collectionView scrollRectToVisible:[_currentEditingCell frame] On 2017/03/16 13:01:06, lpromero wrote: > No action needed: I wonder if using [self.collectionView > scrollToItemAtIndexPath:[self.collectionView > indexPathForCell:_currentEditingCell] > atScrollPosition:UICollectionViewScrollPositionCenteredVertically animated:YES] > would be less brittle. I copied this from autofill_edit_collection_view_controller.mm. Wouldn't it be a weird experience if an already visible text field moved to the middle of the screen once focused? Also if the user is using the accessory view buttons to move up and down between the fields, it's probably enough to just bring the text field into view. https://codereview.chromium.org/2744823003/diff/60001/ios/showcase/payments/s... File ios/showcase/payments/sc_payments_editor_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/showcase/payments/s... ios/showcase/payments/sc_payments_editor_coordinator.mm:22: SectionIdentifierAddress, On 2017/03/16 13:01:07, lpromero wrote: > How will you use PaymentRequestEditViewController in Chrome? Subclasses for > different screens? Yes. please see my reply to your previous comment.
Will it be Parent, Child1, Child2 and you use only Child1 and Child2, or will you use Parent for simple cases and Child for more complex case? I'd rather the first one. Then Showcase can only display Child1 and Child2. But having Parent in Showcase so far is good. https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.h:19: // Field definition for an editor field. Used for building the UI and On 2017/03/16 15:44:22, moe wrote: > On 2017/03/16 13:01:06, lpromero wrote: > > s/an/a > > It should be "an editor...", right? 0_0 Why did I made this comment? https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:341: [self.collectionView scrollRectToVisible:[_currentEditingCell frame] On 2017/03/16 15:44:23, moe wrote: > On 2017/03/16 13:01:06, lpromero wrote: > > No action needed: I wonder if using [self.collectionView > > scrollToItemAtIndexPath:[self.collectionView > > indexPathForCell:_currentEditingCell] > > atScrollPosition:UICollectionViewScrollPositionCenteredVertically > animated:YES] > > would be less brittle. > > I copied this from autofill_edit_collection_view_controller.mm. Wouldn't it be a > weird experience if an already visible text field moved to the middle of the > screen once focused? Also if the user is using the accessory view buttons to > move up and down between the fields, it's probably enough to just bring the text > field into view. Ok. For the up/down button, it doesn't apply here, because this method is not called again, right? Where do you scroll to have the next textfield entirely visible? Is it automatic?
https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/BUILD.gn:92: "//components/autofill/core/browser", Is this used for anything other than autofill::ServerFieldType? We really want to keep the UI layer clean of model types. This is the role of a mediator object that translates the field type to: strings, images, colors, fonts, actions (blocks or selectors). https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/cells/autofill_edit_item.h (right): https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/cells/autofill_edit_item.h:28: @property(nonatomic, assign) autofill::ServerFieldType autofillType; Can you please add a TODO here mentioning 702252 to refactor the dependency on model type?
Hi Louis, PTAL. I did some more refactoring on the "parent" form to ensure the chrome related logic will remain outside the views (including in the child view controllers which I'm working on but haven't submitted yet): 1. Field definitions are now passed to the editor rather than overridden in children. This allows for the parent to be directly used in very simple cases such as the showcase. however, I think in payment request we will end up having to always use child view controllers as there are pretty much no simple editors. 2. Default values and submitted values are communicated through field definitions. 3. Field definitions no longer depend on the model types. 4. Submission and validation are done by delegates (future coordinators). https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/60001/ios/chrome/browser/paym... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:341: [self.collectionView scrollRectToVisible:[_currentEditingCell frame] On 2017/03/16 16:08:08, lpromero wrote: > On 2017/03/16 15:44:23, moe wrote: > > On 2017/03/16 13:01:06, lpromero wrote: > > > No action needed: I wonder if using [self.collectionView > > > scrollToItemAtIndexPath:[self.collectionView > > > indexPathForCell:_currentEditingCell] > > > atScrollPosition:UICollectionViewScrollPositionCenteredVertically > > animated:YES] > > > would be less brittle. > > > > I copied this from autofill_edit_collection_view_controller.mm. Wouldn't it be > a > > weird experience if an already visible text field moved to the middle of the > > screen once focused? Also if the user is using the accessory view buttons to > > move up and down between the fields, it's probably enough to just bring the > text > > field into view. > > Ok. For the up/down button, it doesn't apply here, because this method is not > called again, right? Where do you scroll to have the next textfield entirely > visible? Is it automatic? true about the up/down buttons. If I'm understanding your second question correctly, once the user touches a partially visible textfield (which causes the keyboard to show), this method scrolls to make the textfield entirely visible. https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/paym... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/paym... ios/chrome/browser/payments/BUILD.gn:92: "//components/autofill/core/browser", On 2017/03/16 16:25:50, lpromero wrote: > Is this used for anything other than autofill::ServerFieldType? > We really want to keep the UI layer clean of model types. This is the role of a > mediator object that translates the field type to: strings, images, colors, > fonts, actions (blocks or selectors). After the latest changes, this only exists because there is a translation from NSInteger to autofill::ServerFieldType to set on the AutofillEditItem. Should be able to get rid of this once 702252 is fixed. https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/settings/cells/autofill_edit_item.h (right): https://codereview.chromium.org/2744823003/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/settings/cells/autofill_edit_item.h:28: @property(nonatomic, assign) autofill::ServerFieldType autofillType; On 2017/03/16 16:25:50, lpromero wrote: > Can you please add a TODO here mentioning 702252 to refactor the dependency on > model type? Done.
Description was changed from ========== [Payment Request] Generic edit form http://imgur.com/a/hAsJ7 BUG=602666 ========== to ========== [Payment Request] Generic edit form http://imgur.com/a/KlNeX BUG=602666 ==========
General comments: * Thanks for integrating nicely in Showcase and the cell catalog! * Before following up with the subclass, can you add unit tests and/or EarlGrey tests to the base class? That will help not regressing when integrating the subclass. * "Child view controller" already has a meaning in the context of view controller containment, so prefer to refer to base class/subclass instead of parent and child view controller. Otherwise, this is starting to look great! https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/BUILD.gn:105: "//ios/third_party/material_components_ios", Is this necessary as public_deps? Are there include_dirs we need to forward to this target? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:18: const NSInteger kSectionIdentifierEnumStart = kSectionIdentifierEnumZero + 20; Move declaration to internal header (see below) and definition to implementation file. Moreover, they are not yet used, so I'd advise to add them only in the follow-up CL with subclasses. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:23: @interface EditorField : NSObject Can you move this into its own files? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:83: : CollectionViewController<UITextFieldDelegate, These are implementation details, they can move to the impl file (not even to the internal header I propose below, I think.) https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:92: // The delegate to be called for validating the fields. Add a comment that by default, this object is the validator. Will the validator ever change? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:113: - (void)addOrRemoveErrorMessage:(NSString*)errorMessage Many things in this header are for subclasses use only. Can you create ios/chrome/browser/payments/payment_request_edit_view_controller+internal.h and move all these details there? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:86: UIBarButtonItem* _cancelButton; I don't think you need ivars. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:154: _accessoryView = [[AutofillEditAccessoryView alloc] initWithDelegate:self]; You could also load the model, since it only needs the _fields. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:210: field.sectionIdentifier = static_cast<NSInteger>(sectionIdentifier++); Move the sectionIdentifier incrementation to its own line. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:263: return NO; Seems like this means that when returning from the last text field, the keyboard is not dismissed, right. Is that a UX issue? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:291: if ([cell isKindOfClass:[AutofillEditCell class]]) { Instead of checking the cell class, why don't you do like below, checking the item type? That way it will be on par. Edit: oh, might be related to subclasses of this view controller? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:293: base::mac::ObjCCast<AutofillEditCell>(cell); Use ObjCCastStrict, since you checked the class. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:339: return [MDCCollectionViewCell Seems safe to always return cr_preferred. I am thinking of having this be the default implementation of the CollectionViewController class. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller_actions.h (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller_actions.h:8: // Protocol handling the actions sent in the PaymentRequestEditViewController. s/in/by? https://codereview.chromium.org/2744823003/diff/100001/ios/showcase/payments/... File ios/showcase/payments/sc_payments_editor_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/100001/ios/showcase/payments/... ios/showcase/payments/sc_payments_editor_coordinator.mm:34: [_paymentRequestEditViewController loadModel]; is this needed?
Hi Louis, Please take another look. I will sent out a separate CL to test this before proceeding with the subclass editor CLs. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/BUILD.gn:105: "//ios/third_party/material_components_ios", On 2017/03/22 10:51:33, lpromero wrote: > Is this necessary as public_deps? Are there include_dirs we need to forward to > this target? I remember I had to make this a public dependency to compile the showcase. But it doesn't seem to be the case anymore. I'm not quite sure what include_dirs does. Do I need it? https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:18: const NSInteger kSectionIdentifierEnumStart = kSectionIdentifierEnumZero + 20; On 2017/03/22 10:51:33, lpromero wrote: > Move declaration to internal header (see below) and definition to implementation > file. > > Moreover, they are not yet used, so I'd advise to add them only in the follow-up > CL with subclasses. Yep they're not used. sorry, aftermath of breaking a large CL... As for why they're defined here, for some reason the compiler doesn't like it when these are defined in the implementation. It complains that the "enumerator value is not a constant expression". But i'll deal with it in the follow up subclass CL. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:23: @interface EditorField : NSObject On 2017/03/22 10:51:33, lpromero wrote: > Can you move this into its own files? Done. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:83: : CollectionViewController<UITextFieldDelegate, On 2017/03/22 10:51:33, lpromero wrote: > These are implementation details, they can move to the impl file (not even to > the internal header I propose below, I think.) Done. "In retrospect", the subclasses may need to override methods from UITextFieldDelegate. So it'll have to move to the Internal category. Also, the subclasses's validator delegate will being calling the base class's implementation after doing it's own specific validation so that'll have to remain public. But, we will cross that bridge when we come to it. Moving these to the implementation for now :) https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:92: // The delegate to be called for validating the fields. On 2017/03/22 10:51:33, lpromero wrote: > Add a comment that by default, this object is the validator. Will the validator > ever change? Done. Yes, the base class's default implementation only checks if the field is required since it shouldn't know the logic of validating autofill types. For credit cards or autofill profiles, it'll be the appropriate coordinator who'll be doing the specific field validation. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:113: - (void)addOrRemoveErrorMessage:(NSString*)errorMessage On 2017/03/22 10:51:33, lpromero wrote: > Many things in this header are for subclasses use only. Can you create > ios/chrome/browser/payments/payment_request_edit_view_controller+internal.h and > move all these details there? Done. Everything here is for subclasses use. The reason I made them public was that I remembered from a previous review you suggesting there's no harm in doing so. I personally like the idea of the Internal category. https://codereview.chromium.org/2731293002/diff/1/ios/chrome/browser/ui/setti... https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:86: UIBarButtonItem* _cancelButton; On 2017/03/22 10:51:33, lpromero wrote: > I don't think you need ivars. Done. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:154: _accessoryView = [[AutofillEditAccessoryView alloc] initWithDelegate:self]; On 2017/03/22 10:51:33, lpromero wrote: > You could also load the model, since it only needs the _fields. Good point. For consistency with the other view controllers, I suggest we have the coordinator call that. Moreover, subclasses will need more things so we won't end up calling loadModel twice that way. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:210: field.sectionIdentifier = static_cast<NSInteger>(sectionIdentifier++); On 2017/03/22 10:51:33, lpromero wrote: > Move the sectionIdentifier incrementation to its own line. Done. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:263: return NO; On 2017/03/22 10:51:33, lpromero wrote: > Seems like this means that when returning from the last text field, the keyboard > is not dismissed, right. Is that a UX issue? Oh it probably is. Changed it to dismiss the keyboard when it's the last text field. There are other UX issues that have to be addressed in the future. like the keyboard type or the return key type... https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:291: if ([cell isKindOfClass:[AutofillEditCell class]]) { On 2017/03/22 10:51:33, lpromero wrote: > Instead of checking the cell class, why don't you do like below, checking the > item type? That way it will be on par. > > Edit: oh, might be related to subclasses of this view controller? Actually I forgot to update this after making the latest changes. This can be done in a switch statement since it's the base class that decides the type of the text field item. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:293: base::mac::ObjCCast<AutofillEditCell>(cell); On 2017/03/22 10:51:33, lpromero wrote: > Use ObjCCastStrict, since you checked the class. Acknowledged. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:339: return [MDCCollectionViewCell On 2017/03/22 10:51:33, lpromero wrote: > Seems safe to always return cr_preferred. I am thinking of having this be the > default implementation of the CollectionViewController class. Yep. return MDCCellDefaultOneLineHeight; shouldn't be reached. However, in my opinion it helps to keep NOTREACHED() there to avoid surprises when adding new item types. once the transition is complete I can go back and get rid of these type checks. https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller_actions.h (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller_actions.h:8: // Protocol handling the actions sent in the PaymentRequestEditViewController. On 2017/03/22 10:51:33, lpromero wrote: > s/in/by? Done. https://codereview.chromium.org/2744823003/diff/100001/ios/showcase/payments/... File ios/showcase/payments/sc_payments_editor_coordinator.mm (right): https://codereview.chromium.org/2744823003/diff/100001/ios/showcase/payments/... ios/showcase/payments/sc_payments_editor_coordinator.mm:34: [_paymentRequestEditViewController loadModel]; On 2017/03/22 10:51:33, lpromero wrote: > is this needed? please see my reply to the other related comment.
lgtm https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... File ios/chrome/browser/payments/BUILD.gn (right): https://codereview.chromium.org/2744823003/diff/100001/ios/chrome/browser/pay... ios/chrome/browser/payments/BUILD.gn:105: "//ios/third_party/material_components_ios", On 2017/03/22 17:54:31, moe wrote: > On 2017/03/22 10:51:33, lpromero wrote: > > Is this necessary as public_deps? Are there include_dirs we need to forward to > > this target? > > I remember I had to make this a public dependency to compile the showcase. But > it doesn't seem to be the case anymore. > I'm not quite sure what include_dirs does. Do I need it? I don't think you will need it. You always import Material Components header with the full path ios/third_party/.../MaterialFoo.h, not just MaterialFoo.h. https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:50: // wehn the value of its respective text field is invalid. when https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:58: // The delegate to be called for validating the fields. By default, this object s/this object/the controller, to avoid a doubt in who the object is. https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:59: // is the validator Missing period https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:39: AutofillEditCell* AutofillEditCellForTextField(UITextField* textField) { Nit: add a comment.
The CQ bit was checked by [email protected] to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by [email protected]
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== [Payment Request] Generic edit form http://imgur.com/a/KlNeX BUG=602666 ========== to ========== [Payment Request] Generic edit form http://imgur.com/a/KlNeX BUG=602666 TBR=caitkp@ ==========
Thank you louis! https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.h (right): https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:50: // wehn the value of its respective text field is invalid. On 2017/03/23 12:58:36, lpromero wrote: > when Done. https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:58: // The delegate to be called for validating the fields. By default, this object On 2017/03/23 12:58:35, lpromero wrote: > s/this object/the controller, to avoid a doubt in who the object is. Done. https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.h:59: // is the validator On 2017/03/23 12:58:35, lpromero wrote: > Missing period Done. https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... File ios/chrome/browser/payments/payment_request_edit_view_controller.mm (right): https://codereview.chromium.org/2744823003/diff/120001/ios/chrome/browser/pay... ios/chrome/browser/payments/payment_request_edit_view_controller.mm:39: AutofillEditCell* AutofillEditCellForTextField(UITextField* textField) { On 2017/03/23 12:58:36, lpromero wrote: > Nit: add a comment. Done.
The CQ bit was checked by [email protected]
The patchset sent to the CQ was uploaded after l-g-t-m from [email protected] Link to the patchset: https://codereview.chromium.org/2744823003/#ps160001 (title: "rebase")
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": 160001, "attempt_start_ts": 1490282451120460,
"parent_rev": "a7682c8258c2e04621d791d60ba835f35b90cd1a", "commit_rev":
"33100857169da33f18778cb1f3073e607ee48e27"}
Message was sent while issue was closed.
Description was changed from ========== [Payment Request] Generic edit form http://imgur.com/a/KlNeX BUG=602666 TBR=caitkp@ ========== to ========== [Payment Request] Generic edit form http://imgur.com/a/KlNeX BUG=602666 TBR=caitkp@ Review-Url: https://codereview.chromium.org/2744823003 Cr-Commit-Position: refs/heads/master@{#459111} Committed: https://chromium.googlesource.com/chromium/src/+/33100857169da33f18778cb1f307... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:160001) as https://chromium.googlesource.com/chromium/src/+/33100857169da33f18778cb1f307... |
