Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958823002:
[Payment Request] Incomplete item shouldn't appear as selected when tapped. (Closed)
|
Created:
3 years, 5 months ago by Moe Modified:
3 years, 5 months ago Reviewers:
macourteau CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, marq+watch_chromium.org, noyau+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payment Request] Incomplete item shouldn't appear as selected when tapped.
- Fixes the issue where a checkmark was appearing next to incomplete items
when tapped.
BUG=602666
Review-Url: https://codereview.chromium.org/2958823002
Cr-Commit-Position: refs/heads/master@{#482984}
Committed: https://chromium.googlesource.com/chromium/src/+/a899b4a55e8db762074966412be2980fbd753e07
Patch Set 1 #
Total comments: 7
Patch Set 2 : Addressed comments #
Messages
Total messages: 23 (12 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...
mahmadi@chromium.org changed reviewers: + macourteau@chromium.org
Hi MAC, Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/billing_address_selection_coordinator.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/billing_address_selection_coordinator.mm:142: auto position = nit: const auto https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: didSelectItemAtIndex:index]; Do we still want to notify the delegate of selection even if the item wasn't selected? Would it be worth adding another message, like "didSelectIncompleteItemAtIndex:"? https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/shipping_address_selection_coordinator.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/shipping_address_selection_coordinator.mm:165: auto position = nit: const auto
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...
PTAL? https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/billing_address_selection_coordinator.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/billing_address_selection_coordinator.mm:142: auto position = On 2017/06/27 at 15:27:12, macourteau wrote: > nit: const auto Done. https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: didSelectItemAtIndex:index]; On 2017/06/27 at 15:27:12, macourteau wrote: > Do we still want to notify the delegate of selection even if the item wasn't selected? Would it be worth adding another message, like "didSelectIncompleteItemAtIndex:"? I see your point. I think this call is fine though. The user is really selecting the item. It's the coordinator that then decides if the item is good enough for selection or whether it should be edited. But of course the line is blurry because that information is also needed for display purposes. https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/shipping_address_selection_coordinator.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/shipping_address_selection_coordinator.mm:165: auto position = On 2017/06/27 at 15:27:12, macourteau wrote: > nit: const auto Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ one suggestion https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... File ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm (right): https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: didSelectItemAtIndex:index]; On 2017/06/28 01:57:44, Moe wrote: > On 2017/06/27 at 15:27:12, macourteau wrote: > > Do we still want to notify the delegate of selection even if the item wasn't > selected? Would it be worth adding another message, like > "didSelectIncompleteItemAtIndex:"? > > I see your point. I think this call is fine though. The user is really selecting > the item. It's the coordinator that then decides if the item is good enough for > selection or whether it should be edited. But of course the line is blurry > because that information is also needed for display purposes. Ack. I still feel that the state is confusing: delegate is notified of selection, but the controller does not consider the item selected. It might make sense now that the code is "fresh", but that might be confusing if we end up debugging this code again in e.g. 6 months. Maybe add a parameter to the paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. "selectionIsValid:"? Even if that value was unused at the moment, I think it would make sense, and would make it easy for the delegate to decide to e.g. prompt the user for the missing data, to allow selection of this item. My $0.02 though, you are more familiar with this code, so feel free to ignore my suggestions if they don't make sense.
On 2017/06/28 at 13:59:14, macourteau wrote: > lgtm w/ one suggestion > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > File ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm (right): > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: didSelectItemAtIndex:index]; > On 2017/06/28 01:57:44, Moe wrote: > > On 2017/06/27 at 15:27:12, macourteau wrote: > > > Do we still want to notify the delegate of selection even if the item wasn't > > selected? Would it be worth adding another message, like > > "didSelectIncompleteItemAtIndex:"? > > > > I see your point. I think this call is fine though. The user is really selecting > > the item. It's the coordinator that then decides if the item is good enough for > > selection or whether it should be edited. But of course the line is blurry > > because that information is also needed for display purposes. > > Ack. I still feel that the state is confusing: delegate is notified of selection, but the controller does not consider the item selected. It might make sense now that the code is "fresh", but that might be confusing if we end up debugging this code again in e.g. 6 months. > > Maybe add a parameter to the paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. "selectionIsValid:"? Even if that value was unused at the moment, I think it would make sense, and would make it easy for the delegate to decide to e.g. prompt the user for the missing data, to allow selection of this item. > > My $0.02 though, you are more familiar with this code, so feel free to ignore my suggestions if they don't make sense. Ack. My argument is that the view controller shouldn't make that decision. It should only notify its delegate of the action of selection by the user. The delegate, in this case the coordinator, should have all the information necessary to mark that item as selected or open another screen for editing missing information. Ideally the coordinator then asks the view controller to update itself based on the new selection or lack thereof, so that the checkmarks get updated. The problem is that doing so causes the ink effect on tapping not to be seen properly. so we use this hack of updating the items visually in the view controller based on the |complete| property of the item. I don't think the view controller's role (read "hack") should go beyond this.
The CQ bit was checked by mahmadi@chromium.org
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": 20001, "attempt_start_ts": 1498661088024490,
"parent_rev": "b23adee86fdfa5d15dc82f0753ae13aed5c6e13a", "commit_rev":
"a899b4a55e8db762074966412be2980fbd753e07"}
Message was sent while issue was closed.
Description was changed from ========== [Payment Request] Incomplete item shouldn't appear as selected when tapped. - Fixes the issue where a checkmark was appearing next to incomplete items when tapped. BUG=602666 ========== to ========== [Payment Request] Incomplete item shouldn't appear as selected when tapped. - Fixes the issue where a checkmark was appearing next to incomplete items when tapped. BUG=602666 Review-Url: https://codereview.chromium.org/2958823002 Cr-Commit-Position: refs/heads/master@{#482984} Committed: https://chromium.googlesource.com/chromium/src/+/a899b4a55e8db762074966412be2... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/a899b4a55e8db762074966412be2...
Message was sent while issue was closed.
On 2017/06/28 14:44:43, Moe wrote: > On 2017/06/28 at 13:59:14, macourteau wrote: > > lgtm w/ one suggestion > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > File > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm > (right): > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: > didSelectItemAtIndex:index]; > > On 2017/06/28 01:57:44, Moe wrote: > > > On 2017/06/27 at 15:27:12, macourteau wrote: > > > > Do we still want to notify the delegate of selection even if the item > wasn't > > > selected? Would it be worth adding another message, like > > > "didSelectIncompleteItemAtIndex:"? > > > > > > I see your point. I think this call is fine though. The user is really > selecting > > > the item. It's the coordinator that then decides if the item is good enough > for > > > selection or whether it should be edited. But of course the line is blurry > > > because that information is also needed for display purposes. > > > > Ack. I still feel that the state is confusing: delegate is notified of > selection, but the controller does not consider the item selected. It might make > sense now that the code is "fresh", but that might be confusing if we end up > debugging this code again in e.g. 6 months. > > > > Maybe add a parameter to the > paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. > "selectionIsValid:"? Even if that value was unused at the moment, I think it > would make sense, and would make it easy for the delegate to decide to e.g. > prompt the user for the missing data, to allow selection of this item. > > > > My $0.02 though, you are more familiar with this code, so feel free to ignore > my suggestions if they don't make sense. > > Ack. My argument is that the view controller shouldn't make that decision. It > should only notify its delegate of the action of selection by the user. The > delegate, in this case the coordinator, should have all the information > necessary to mark that item as selected or open another screen for editing > missing information. Ideally the coordinator then asks the view controller to > update itself based on the new selection or lack thereof, so that the checkmarks > get updated. The problem is that doing so causes the ink effect on tapping not > to be seen properly. so we use this hack of updating the items visually in the > view controller based on the |complete| property of the item. I don't think the > view controller's role (read "hack") should go beyond this. So maybe it would make sense for the view controller to call the selector on the delegate, and have that selector return a boolean indicating whether it can be selected or not? That way, the logic is in one place, and the view controller doesn't need to take any decisions.
Message was sent while issue was closed.
On 2017/06/28 at 14:50:29, macourteau wrote: > On 2017/06/28 14:44:43, Moe wrote: > > On 2017/06/28 at 13:59:14, macourteau wrote: > > > lgtm w/ one suggestion > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > File > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm > > (right): > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: > > didSelectItemAtIndex:index]; > > > On 2017/06/28 01:57:44, Moe wrote: > > > > On 2017/06/27 at 15:27:12, macourteau wrote: > > > > > Do we still want to notify the delegate of selection even if the item > > wasn't > > > > selected? Would it be worth adding another message, like > > > > "didSelectIncompleteItemAtIndex:"? > > > > > > > > I see your point. I think this call is fine though. The user is really > > selecting > > > > the item. It's the coordinator that then decides if the item is good enough > > for > > > > selection or whether it should be edited. But of course the line is blurry > > > > because that information is also needed for display purposes. > > > > > > Ack. I still feel that the state is confusing: delegate is notified of > > selection, but the controller does not consider the item selected. It might make > > sense now that the code is "fresh", but that might be confusing if we end up > > debugging this code again in e.g. 6 months. > > > > > > Maybe add a parameter to the > > paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. > > "selectionIsValid:"? Even if that value was unused at the moment, I think it > > would make sense, and would make it easy for the delegate to decide to e.g. > > prompt the user for the missing data, to allow selection of this item. > > > > > > My $0.02 though, you are more familiar with this code, so feel free to ignore > > my suggestions if they don't make sense. > > > > Ack. My argument is that the view controller shouldn't make that decision. It > > should only notify its delegate of the action of selection by the user. The > > delegate, in this case the coordinator, should have all the information > > necessary to mark that item as selected or open another screen for editing > > missing information. Ideally the coordinator then asks the view controller to > > update itself based on the new selection or lack thereof, so that the checkmarks > > get updated. The problem is that doing so causes the ink effect on tapping not > > to be seen properly. so we use this hack of updating the items visually in the > > view controller based on the |complete| property of the item. I don't think the > > view controller's role (read "hack") should go beyond this. > > So maybe it would make sense for the view controller to call the selector on the delegate, and have that selector return a boolean indicating whether it can be selected or not? That way, the logic is in one place, and the view controller doesn't need to take any decisions. Are you suggesting we change if (newSelectedItem.isComplete) {} to if ([self.delegate canSelectItemAtIndex:index]) {} that's certainly possible. but it doesn't save the view controller from deciding whether to update its cells or not. Am I missing something?
Message was sent while issue was closed.
On 2017/06/28 15:02:44, Moe wrote: > On 2017/06/28 at 14:50:29, macourteau wrote: > > On 2017/06/28 14:44:43, Moe wrote: > > > On 2017/06/28 at 13:59:14, macourteau wrote: > > > > lgtm w/ one suggestion > > > > > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > > File > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm > > > (right): > > > > > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > > > > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: > > > didSelectItemAtIndex:index]; > > > > On 2017/06/28 01:57:44, Moe wrote: > > > > > On 2017/06/27 at 15:27:12, macourteau wrote: > > > > > > Do we still want to notify the delegate of selection even if the item > > > wasn't > > > > > selected? Would it be worth adding another message, like > > > > > "didSelectIncompleteItemAtIndex:"? > > > > > > > > > > I see your point. I think this call is fine though. The user is really > > > selecting > > > > > the item. It's the coordinator that then decides if the item is good > enough > > > for > > > > > selection or whether it should be edited. But of course the line is > blurry > > > > > because that information is also needed for display purposes. > > > > > > > > Ack. I still feel that the state is confusing: delegate is notified of > > > selection, but the controller does not consider the item selected. It might > make > > > sense now that the code is "fresh", but that might be confusing if we end up > > > debugging this code again in e.g. 6 months. > > > > > > > > Maybe add a parameter to the > > > paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. > > > "selectionIsValid:"? Even if that value was unused at the moment, I think it > > > would make sense, and would make it easy for the delegate to decide to e.g. > > > prompt the user for the missing data, to allow selection of this item. > > > > > > > > My $0.02 though, you are more familiar with this code, so feel free to > ignore > > > my suggestions if they don't make sense. > > > > > > Ack. My argument is that the view controller shouldn't make that decision. > It > > > should only notify its delegate of the action of selection by the user. The > > > delegate, in this case the coordinator, should have all the information > > > necessary to mark that item as selected or open another screen for editing > > > missing information. Ideally the coordinator then asks the view controller > to > > > update itself based on the new selection or lack thereof, so that the > checkmarks > > > get updated. The problem is that doing so causes the ink effect on tapping > not > > > to be seen properly. so we use this hack of updating the items visually in > the > > > view controller based on the |complete| property of the item. I don't think > the > > > view controller's role (read "hack") should go beyond this. > > > > So maybe it would make sense for the view controller to call the selector on > the delegate, and have that selector return a boolean indicating whether it can > be selected or not? That way, the logic is in one place, and the view controller > doesn't need to take any decisions. > > Are you suggesting we change if (newSelectedItem.isComplete) {} to if > ([self.delegate canSelectItemAtIndex:index]) {} > that's certainly possible. but it doesn't save the view controller from deciding > whether to update its cells or not. Am I missing something? Yes. :) Or more simply: if ([self.delegate paymentRequestSelectorViewController:self didSelectItemAtIndex:index]) { // ... } The deciding logic in this case is whether the item is complete or not. It makes sense to keep the contents of the "if" itself in the view controller, since it's all view-related stuff. The "if" condition itself, though, is some logic based on the contents of the view controller's items, so that is the part that should be moved out, IMHO.
Message was sent while issue was closed.
On 2017/06/28 at 15:08:57, macourteau wrote: > On 2017/06/28 15:02:44, Moe wrote: > > On 2017/06/28 at 14:50:29, macourteau wrote: > > > On 2017/06/28 14:44:43, Moe wrote: > > > > On 2017/06/28 at 13:59:14, macourteau wrote: > > > > > lgtm w/ one suggestion > > > > > > > > > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > > > File > > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm > > > > (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/2958823002/diff/1/ios/chrome/browser/ui/payme... > > > > > > > > > > > ios/chrome/browser/ui/payments/payment_request_selector_view_controller.mm:288: > > > > didSelectItemAtIndex:index]; > > > > > On 2017/06/28 01:57:44, Moe wrote: > > > > > > On 2017/06/27 at 15:27:12, macourteau wrote: > > > > > > > Do we still want to notify the delegate of selection even if the item > > > > wasn't > > > > > > selected? Would it be worth adding another message, like > > > > > > "didSelectIncompleteItemAtIndex:"? > > > > > > > > > > > > I see your point. I think this call is fine though. The user is really > > > > selecting > > > > > > the item. It's the coordinator that then decides if the item is good > > enough > > > > for > > > > > > selection or whether it should be edited. But of course the line is > > blurry > > > > > > because that information is also needed for display purposes. > > > > > > > > > > Ack. I still feel that the state is confusing: delegate is notified of > > > > selection, but the controller does not consider the item selected. It might > > make > > > > sense now that the code is "fresh", but that might be confusing if we end up > > > > debugging this code again in e.g. 6 months. > > > > > > > > > > Maybe add a parameter to the > > > > paymentRequestSelectorViewController:didSelectItemAtIndex: selector, e.g. > > > > "selectionIsValid:"? Even if that value was unused at the moment, I think it > > > > would make sense, and would make it easy for the delegate to decide to e.g. > > > > prompt the user for the missing data, to allow selection of this item. > > > > > > > > > > My $0.02 though, you are more familiar with this code, so feel free to > > ignore > > > > my suggestions if they don't make sense. > > > > > > > > Ack. My argument is that the view controller shouldn't make that decision. > > It > > > > should only notify its delegate of the action of selection by the user. The > > > > delegate, in this case the coordinator, should have all the information > > > > necessary to mark that item as selected or open another screen for editing > > > > missing information. Ideally the coordinator then asks the view controller > > to > > > > update itself based on the new selection or lack thereof, so that the > > checkmarks > > > > get updated. The problem is that doing so causes the ink effect on tapping > > not > > > > to be seen properly. so we use this hack of updating the items visually in > > the > > > > view controller based on the |complete| property of the item. I don't think > > the > > > > view controller's role (read "hack") should go beyond this. > > > > > > So maybe it would make sense for the view controller to call the selector on > > the delegate, and have that selector return a boolean indicating whether it can > > be selected or not? That way, the logic is in one place, and the view controller > > doesn't need to take any decisions. > > > > Are you suggesting we change if (newSelectedItem.isComplete) {} to if > > ([self.delegate canSelectItemAtIndex:index]) {} > > that's certainly possible. but it doesn't save the view controller from deciding > > whether to update its cells or not. Am I missing something? > > Yes. :) Or more simply: > > if ([self.delegate paymentRequestSelectorViewController:self didSelectItemAtIndex:index]) { > // ... > } > > The deciding logic in this case is whether the item is complete or not. It makes sense to keep the contents of the "if" itself in the view controller, since it's all view-related stuff. The "if" condition itself, though, is some logic based on the contents of the view controller's items, so that is the part that should be moved out, IMHO. I see and I agree. This should make things better now: https://codereview.chromium.org/2956323002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
