Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2625693002:
Suggestions UI - expandable item (Closed)
DescriptionSuggestions UI - expandable item
BUG=679369
Review-Url: https://codereview.chromium.org/2625693002
Cr-Commit-Position: refs/heads/master@{#444056}
Committed: https://chromium.googlesource.com/chromium/src/+/9a26ea51a7b89d72ea18a9bf3583440a63be766d
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 18
Patch Set 3 : Address some comments #Patch Set 4 : Add tests #
Total comments: 2
Patch Set 5 : Use Audience #
Total comments: 10
Patch Set 6 : Address comments #Patch Set 7 : Add comments #
Total comments: 8
Patch Set 8 : Update tests #
Total comments: 38
Patch Set 9 : Address comments #
Total comments: 1
Patch Set 10 : Cleanup #
Total comments: 6
Patch Set 11 : Address comments #Messages
Total messages: 24 (6 generated)
gambard@chromium.org changed reviewers: + lpromero@chromium.org, marq@chromium.org
PTAL. Split 4/4 of https://codereview.chromium.org/2618343002/
Test file for the item. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_data_source.mm (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_data_source.mm:55: title:@"Title of an Expandable Article" Can you add a bug to track removing these? I guess to be functional, you'll need to remove these at some point, but it would be good to track it. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: @property(nonatomic, assign) UICollectionView* collectionView; Don't depend on the collection like this. Have a delegate on the cell is that notified of UI events, then the delegate can do this. Keep the item out of the loop. You can set the delegate in ~cellForItemAtIndexPath. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:35: @property(nonatomic, assign) SuggestionsExpandableItem* item; I'd remove this and keep the item just as a data holder. Collection and cell can talk to each other in ~cellForItemAtIndexPath and delegate callbacks. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:51: _title = title; Idem about copy and properties. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:119: _titleLabel = [[UILabel alloc] initWithFrame:CGRectZero]; Nit: I usually use just init. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:129: [_expandButton setTitle:@"See more" forState:UIControlStateNormal]; Add a localized string or a bug to track. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:131: [_interactionButton setTitle:@"Less Interactions" Idem. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:190: _title = title; Is that needed?
Thanks, PTAL. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_data_source.mm (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_data_source.mm:55: title:@"Title of an Expandable Article" On 2017/01/11 13:19:20, lpromero wrote: > Can you add a bug to track removing these? I guess to be functional, you'll need > to remove these at some point, but it would be good to track it. Well the bug for creating the suggestions UI should be sufficient. This clearly prevent a correct UI to be implemented. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: @property(nonatomic, assign) UICollectionView* collectionView; On 2017/01/11 13:19:20, lpromero wrote: > Don't depend on the collection like this. Have a delegate on the cell is that > notified of UI events, then the delegate can do this. > Keep the item out of the loop. You can set the delegate in > ~cellForItemAtIndexPath. It means inspecting the type of every cell in the collection to check if they are SuggestionsExpandableCell. This seems a bit extreme. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:35: @property(nonatomic, assign) SuggestionsExpandableItem* item; On 2017/01/11 13:19:20, lpromero wrote: > I'd remove this and keep the item just as a data holder. Collection and cell can > talk to each other in ~cellForItemAtIndexPath and delegate callbacks. The item needs to know if the cell has been expanded or not to adjust the height (calling expand or retract on the sizing cell). https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:51: _title = title; On 2017/01/11 13:19:20, lpromero wrote: > Idem about copy and properties. Done. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:119: _titleLabel = [[UILabel alloc] initWithFrame:CGRectZero]; On 2017/01/11 13:19:20, lpromero wrote: > Nit: I usually use just init. Acknowledged. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:129: [_expandButton setTitle:@"See more" forState:UIControlStateNormal]; On 2017/01/11 13:19:20, lpromero wrote: > Add a localized string or a bug to track. Done. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:131: [_interactionButton setTitle:@"Less Interactions" On 2017/01/11 13:19:20, lpromero wrote: > Idem. Done. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:190: _title = title; On 2017/01/11 13:19:20, lpromero wrote: > Is that needed? Done.
https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: @property(nonatomic, assign) UICollectionView* collectionView; On 2017/01/12 14:39:40, gambard wrote: > On 2017/01/11 13:19:20, lpromero wrote: > > Don't depend on the collection like this. Have a delegate on the cell is that > > notified of UI events, then the delegate can do this. > > Keep the item out of the loop. You can set the delegate in > > ~cellForItemAtIndexPath. > > It means inspecting the type of every cell in the collection to check if they > are SuggestionsExpandableCell. This seems a bit extreme. Don't check the cell type, check the ItemType for the index path. That's what we always do in these -collectionView:cellForItemAtIndexPath: methods. Having the item depend on the collection view is a show-stopper. It is the controller's job to handle the relationship between item and cell, even if that means more code in the controller. We sometimes call that "dumb item", or "dumb cell", where they don't communicate. https://codereview.chromium.org/2625693002/diff/20001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:35: @property(nonatomic, assign) SuggestionsExpandableItem* item; On 2017/01/12 14:39:40, gambard wrote: > On 2017/01/11 13:19:20, lpromero wrote: > > I'd remove this and keep the item just as a data holder. Collection and cell > can > > talk to each other in ~cellForItemAtIndexPath and delegate callbacks. > > The item needs to know if the cell has been expanded or not to adjust the height > (calling expand or retract on the sizing cell). The item should hold that knowledge. In its public interface. Ideally, the item should be sufficient to recreate a cell from the ground up. https://codereview.chromium.org/2625693002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:40: BOOL isExpanded; s/isExpanded/_isExpanded.
Thanks. I have changed the collectionView interaction to an audience protocol. PTAL. https://codereview.chromium.org/2625693002/diff/60001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/60001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:40: BOOL isExpanded; On 2017/01/12 16:44:39, lpromero wrote: > s/isExpanded/_isExpanded. Done.
https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:65: expandableItem.audience = collectionViewController; Please call it a delegate, as it is the idiom on iOS. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:37: - (void)expand:(SuggestionsExpandableCell*)cell; A isExpanded property needs to appear in the interface. Then you can remove these and in the collection view controller, you do: item.expanded = YES; [self reconfigureCellsForItems:@[ item ]]; This will call in turn the expand/retract methods on the cell. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:71: - (void)expand:(SuggestionsExpandableCell*)cell { You can remove these two. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:32: Can you add a test for the delegate and the expansion/retraction? https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:69: [self interact:^(SuggestionsExpandableItem* item, See streamlined code in my comment above.
Thanks, PTAL. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.mm:65: expandableItem.audience = collectionViewController; On 2017/01/13 17:35:48, lpromero wrote: > Please call it a delegate, as it is the idiom on iOS. Done. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:37: - (void)expand:(SuggestionsExpandableCell*)cell; On 2017/01/13 17:35:48, lpromero wrote: > A isExpanded property needs to appear in the interface. Then you can remove > these and in the collection view controller, you do: > > item.expanded = YES; > [self reconfigureCellsForItems:@[ item ]]; > > This will call in turn the expand/retract methods on the cell. Done. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:71: - (void)expand:(SuggestionsExpandableCell*)cell { On 2017/01/13 17:35:48, lpromero wrote: > You can remove these two. Done. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:32: On 2017/01/13 17:35:48, lpromero wrote: > Can you add a test for the delegate and the expansion/retraction? Done. https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/80001/ios/chrome/browser/ui/s... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:69: [self interact:^(SuggestionsExpandableItem* item, On 2017/01/13 17:35:48, lpromero wrote: > See streamlined code in my comment above. Done.
lgtm https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm (right): https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:13: @interface SuggestionsExpandableCellTest : SuggestionsExpandableCell It should be called TestSuggestionsExpandableCell. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:18: - (void)expand; Make a category in the header for SuggestionsExpandableCell for testing only. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:29: self.expandCalled = YES; Call the parent implementation. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:33: self.retractCalled = YES; Idem.
Thanks. marq@ do you want to take a look? https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm (right): https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:13: @interface SuggestionsExpandableCellTest : SuggestionsExpandableCell On 2017/01/16 11:35:18, lpromero wrote: > It should be called TestSuggestionsExpandableCell. Done. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:18: - (void)expand; On 2017/01/16 11:35:18, lpromero wrote: > Make a category in the header for SuggestionsExpandableCell for testing only. Done. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:29: self.expandCalled = YES; On 2017/01/16 11:35:18, lpromero wrote: > Call the parent implementation. Done. https://codereview.chromium.org/2625693002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item_unittest.mm:33: self.retractCalled = YES; On 2017/01/16 11:35:18, lpromero wrote: > Idem. Done.
https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:15: // the expansion/retractation of the cell. Perfer 'collapse' as the opposite to 'expand' instead of 'retract' ('retract' usually applies to something long and narrow, like a rope or a telescoping rod). https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: // displayed only when the article is expanded. |type| is the type of the item. Does |type| vary orthogonally to the expandable/non-expandable distinction? Are all expandable articles of a certain set of types? https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:37: // Delegate for the configured cells. each CollectionViewItem configures only a single cell, right? So 'cell' rather than 'cells'. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:70: BOOL _isExpanded; Boolean vars shouldn't use 'is'; boolean getters should. So if this were a property is would be @property (nonatomic, getter=isExpanded) BOOL expanded, the ivar would be _expanded, and it could be accessed via any of: _expanded self.expanded [self isExpanded] _expanded = NO self.expanded = NO; [self setExpanded:NO]; https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:107: [_expandButton addTarget:self Why not target:nil? https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:108: action:@selector(expandPressed) I prefer action methods to not depend on a particular UI representation. Ideally these would just be expand: and retract: (or collapse:). https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:114: [imageContainer addSubview:_imageView]; Can you explain in comments why the imageContainer is necessary? https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:150: #pragma mark - Private Comments on all private methods. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:153: [self.delegate expandCell:self]; Here's something to consider: If this instead went up the responder chain and sent the sender with it (so it was expand:(id)sender), the collection view controller could catch it, cast |sender| to a UIView, and then find the UICollectionViewCell* that's an ancestor of the cell in the view hierarchy. The upside is you don't need to set a delegate at all, and the control better leverages the target/action pattern. The downside is that you have the view controller inspecting a subview's view hierarchy. I personally don't think that's a problem here, because it can be done without the view controller knowing what kind of cell it is, and I think this kind of thing is why the sender can be passed in the target/action pattern. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:169: [NSLayoutConstraint activateConstraints:@[ This only works because _detailLabel and _interactionMenu are being removed from their superview in -retract. If the implementation were changed to set the views to hidden=YES instead of removing the view, each call of expand or retract would generate new constraints. I think it would be a bit safer to keep arrays of additional constraints for expanded and collapsed states and enable/disable them when toggling. Also the general procedure when adding and removing constraints is that it's safe to remove (deactivate) constraints at any time, but it's best to add new constraints in -updateConstraints. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:23: // Applies |interaction| to the item corresponding to |cell| if it is a |interaction| isn't defined. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { As soon as you start checking for class membership at runtime, you should make whatever part of the API you need into a protocol, and instead check for that. You will still need to cast into id<ExpandableItem>, but protocols are less brittle than explicit casts. (note that ObjCCast doesn't work with protocols). https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:101: expandableItem.expanded = expand; lpromero@ -- The cycle of: - Collection item changes - CollectionViewController reconfigures cells for that item (looking up its section in order to do so) - CollectionViewLayout is invalidated Seems clunky, but also one that's likely to be repeated. It seems like once the item is part of the collection view's model, changing it should be able to trigger a relayout. Ideally this code should just be: [UIView animateWithDuration:duration animations:^{expandableItem.expanded = expand;}]; And all of the lookup, cell reconfiguration, and layout invalidation follows implicitly from there. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:106: animateWithDuration:1 use a constant for animation duration. one second is a fairly long animation; 0.35 is typical for most UI transitions.
Thanks. lpromero, can you check the comments? https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:15: // the expansion/retractation of the cell. On 2017/01/16 17:16:41, marq wrote: > Perfer 'collapse' as the opposite to 'expand' instead of 'retract' ('retract' > usually applies to something long and narrow, like a rope or a telescoping rod). Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: // displayed only when the article is expanded. |type| is the type of the item. On 2017/01/16 17:16:41, marq wrote: > Does |type| vary orthogonally to the expandable/non-expandable distinction? Are > all expandable articles of a certain set of types? It is independent. All expandable items can have the same type or different type. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:37: // Delegate for the configured cells. On 2017/01/16 17:16:41, marq wrote: > each CollectionViewItem configures only a single cell, right? So 'cell' rather > than 'cells'. The item configure the cell for displaying on the collection view and the cell for the sizing. Nothing on the API states that it would configure only one cell. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:70: BOOL _isExpanded; On 2017/01/16 17:16:41, marq wrote: > Boolean vars shouldn't use 'is'; boolean getters should. So if this were a > property is would be @property (nonatomic, getter=isExpanded) BOOL expanded, the > ivar would be _expanded, and it could be accessed via any of: > > _expanded > self.expanded > [self isExpanded] > _expanded = NO > self.expanded = NO; > [self setExpanded:NO]; > Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:107: [_expandButton addTarget:self On 2017/01/16 17:16:41, marq wrote: > Why not target:nil? Because it does not work. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:108: action:@selector(expandPressed) On 2017/01/16 17:16:41, marq wrote: > I prefer action methods to not depend on a particular UI representation. Ideally > these would just be expand: and retract: (or collapse:). Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:114: [imageContainer addSubview:_imageView]; On 2017/01/16 17:16:41, marq wrote: > Can you explain in comments why the imageContainer is necessary? Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:150: #pragma mark - Private On 2017/01/16 17:16:41, marq wrote: > Comments on all private methods. Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:169: [NSLayoutConstraint activateConstraints:@[ On 2017/01/16 17:16:41, marq wrote: > This only works because _detailLabel and _interactionMenu are being removed from > their superview in -retract. If the implementation were changed to set the views > to hidden=YES instead of removing the view, each call of expand or retract would > generate new constraints. > > I think it would be a bit safer to keep arrays of additional constraints for > expanded and collapsed states and enable/disable them when toggling. > > Also the general procedure when adding and removing constraints is that it's > safe to remove (deactivate) constraints at any time, but it's best to add new > constraints in -updateConstraints. Done. I might have missread https://developer.apple.com/reference/uikit/uiview/1622512-updateconstraints?... but it states: "It is almost always cleaner and easier to update a constraint immediately after the affecting change has occurred. For example, if you want to change a constraint in response to a button tap, make that change directly in the button’s action method." https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:23: // Applies |interaction| to the item corresponding to |cell| if it is a On 2017/01/16 17:16:42, marq wrote: > |interaction| isn't defined. Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { On 2017/01/16 17:16:42, marq wrote: > As soon as you start checking for class membership at runtime, you should make > whatever part of the API you need into a protocol, and instead check for that. > You will still need to cast into id<ExpandableItem>, but protocols are less > brittle than explicit casts. (note that ObjCCast doesn't work with protocols). Done. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:106: animateWithDuration:1 On 2017/01/16 17:16:42, marq wrote: > use a constant for animation duration. > > one second is a fairly long animation; 0.35 is typical for most UI transitions. Done.
https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h:28: // displayed only when the article is expanded. |type| is the type of the item. On 2017/01/17 09:59:00, gambard wrote: > On 2017/01/16 17:16:41, marq wrote: > > Does |type| vary orthogonally to the expandable/non-expandable distinction? > Are > > all expandable articles of a certain set of types? > > It is independent. All expandable items can have the same type or different > type. And in the other direction, the group of expandable items and the group of non-expandable items don't share types. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { This should just check the item type. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:101: expandableItem.expanded = expand; On 2017/01/16 17:16:42, marq wrote: > lpromero@ -- The cycle of: > > - Collection item changes > - CollectionViewController reconfigures cells for that item (looking up its > section in order to do so) > - CollectionViewLayout is invalidated > > Seems clunky, but also one that's likely to be repeated. It seems like once the > item is part of the collection view's model, changing it should be able to > trigger a relayout. Ideally this code should just be: > > [UIView animateWithDuration:duration animations:^{expandableItem.expanded = > expand;}]; > > And all of the lookup, cell reconfiguration, and layout invalidation follows > implicitly from there. It is a design decision to have the controller explicitly do this work. We don't want the model, even less so the item, depend on the collection view, and we don't want to construct a paradigm we will one day need to workaround (like QTM collections). The layout invalidation for example is required here because the cells are resized, but is usually not necessary.
Thanks. marq@, PTAL. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:153: [self.delegate expandCell:self]; On 2017/01/16 17:16:41, marq wrote: > Here's something to consider: If this instead went up the responder chain and > sent the sender with it (so it was expand:(id)sender), the collection view > controller could catch it, cast |sender| to a UIView, and then find the > UICollectionViewCell* that's an ancestor of the cell in the view hierarchy. > > The upside is you don't need to set a delegate at all, and the control better > leverages the target/action pattern. > > The downside is that you have the view controller inspecting a subview's view > hierarchy. I personally don't think that's a problem here, because it can be > done without the view controller knowing what kind of cell it is, and I think > this kind of thing is why the sender can be passed in the target/action pattern. I don't like having to inspect all the cells to determine the one containing the |sender|. I think this is better. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { On 2017/01/17 10:38:49, lpromero wrote: > This should just check the item type. As I am checking if it conforms to protocol, the itemType check is not needed for now. If I have different classes conforming to the protocol, I would use the itemType to discriminate.
LGTM with one name change (and a bunch of tangential comments). https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:153: [self.delegate expandCell:self]; On 2017/01/17 11:55:29, gambard wrote: > On 2017/01/16 17:16:41, marq wrote: > > Here's something to consider: If this instead went up the responder chain and > > sent the sender with it (so it was expand:(id)sender), the collection view > > controller could catch it, cast |sender| to a UIView, and then find the > > UICollectionViewCell* that's an ancestor of the cell in the view hierarchy. > > > > The upside is you don't need to set a delegate at all, and the control better > > leverages the target/action pattern. > > > > The downside is that you have the view controller inspecting a subview's view > > hierarchy. I personally don't think that's a problem here, because it can be > > done without the view controller knowing what kind of cell it is, and I think > > this kind of thing is why the sender can be passed in the target/action > pattern. > > I don't like having to inspect all the cells to determine the one containing the > |sender|. I think this is better. That's fine, although you won't have to inspect all of the cells -- you would walk up the view hierarchy from |sender| until you found a containing cell, which should only be a couple of steps. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:169: [NSLayoutConstraint activateConstraints:@[ On 2017/01/17 09:59:00, gambard wrote: > On 2017/01/16 17:16:41, marq wrote: > > This only works because _detailLabel and _interactionMenu are being removed > from > > their superview in -retract. If the implementation were changed to set the > views > > to hidden=YES instead of removing the view, each call of expand or retract > would > > generate new constraints. > > > > I think it would be a bit safer to keep arrays of additional constraints for > > expanded and collapsed states and enable/disable them when toggling. > > > > Also the general procedure when adding and removing constraints is that it's > > safe to remove (deactivate) constraints at any time, but it's best to add new > > constraints in -updateConstraints. > > Done. > I might have missread > https://developer.apple.com/reference/uikit/uiview/1622512-updateconstraints?... > but it states: > "It is almost always cleaner and easier to update a constraint immediately after > the affecting change has occurred. For example, if you want to change a > constraint in response to a button tap, make that change directly in the > button’s action method." Looks like the guidelines have changed a bit, although they do still say to centralize adding new constraints in -updateConstraints for best performance. Also 'updating' a constraint can just be changing a property (usually |constant|) of an existing constraint. That's fine to do anywhere. I haven't had a chance to closely evaluate all the approaches to updating constraints in complex views under iOS 9+, so I actually don't know the advantages of keeping constraint activations in -updateConstraints. So I'll fall back on the general principle: use the approach that makes the code cleanest, and then if you identify a performance issue, make adjustments. Which is another way of saying you can ignore the last paragraph of my earlier comment ;-) https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { On 2017/01/17 11:55:29, gambard wrote: > On 2017/01/17 10:38:49, lpromero wrote: > > This should just check the item type. > > As I am checking if it conforms to protocol, the itemType check is not needed > for now. If I have different classes conforming to the protocol, I would use the > itemType to discriminate. Part of the benefit of the protocol is that you don't care what the class of the conforming object is. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:101: expandableItem.expanded = expand; On 2017/01/17 10:38:49, lpromero wrote: > On 2017/01/16 17:16:42, marq wrote: > > lpromero@ -- The cycle of: > > > > - Collection item changes > > - CollectionViewController reconfigures cells for that item (looking up its > > section in order to do so) > > - CollectionViewLayout is invalidated > > > > Seems clunky, but also one that's likely to be repeated. It seems like once > the > > item is part of the collection view's model, changing it should be able to > > trigger a relayout. Ideally this code should just be: > > > > [UIView animateWithDuration:duration animations:^{expandableItem.expanded = > > expand;}]; > > > > And all of the lookup, cell reconfiguration, and layout invalidation follows > > implicitly from there. > > It is a design decision to have the controller explicitly do this work. We don't > want the model, even less so the item, depend on the collection view, and we > don't want to construct a paradigm we will one day need to workaround (like QTM > collections). > > The layout invalidation for example is required here because the cells are > resized, but is usually not necessary. I was thinking more of some way for the view item to signal that its cells were now dirty without necessarily creating a strong dependency from the model to the controller itself, but that's all out of the scope of this CL. https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h (right): https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h:10: // Protocol allowing an item to be expanded. Specify 'CollectionViewItem' https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h:11: @protocol SuggestionsExpandableArticle Make it less specific: ExpandableItem. https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:71: NSArray* _expandedConstraints; NSArray<NSLayoutConstraint*>*
Thanks! https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:153: [self.delegate expandCell:self]; On 2017/01/17 13:42:16, marq wrote: > On 2017/01/17 11:55:29, gambard wrote: > > On 2017/01/16 17:16:41, marq wrote: > > > Here's something to consider: If this instead went up the responder chain > and > > > sent the sender with it (so it was expand:(id)sender), the collection view > > > controller could catch it, cast |sender| to a UIView, and then find the > > > UICollectionViewCell* that's an ancestor of the cell in the view hierarchy. > > > > > > The upside is you don't need to set a delegate at all, and the control > better > > > leverages the target/action pattern. > > > > > > The downside is that you have the view controller inspecting a subview's > view > > > hierarchy. I personally don't think that's a problem here, because it can be > > > done without the view controller knowing what kind of cell it is, and I > think > > > this kind of thing is why the sender can be passed in the target/action > > pattern. > > > > I don't like having to inspect all the cells to determine the one containing > the > > |sender|. I think this is better. > > That's fine, although you won't have to inspect all of the cells -- you would > walk up the view hierarchy from |sender| until you found a containing cell, > which should only be a couple of steps. Acknowledged. https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:169: [NSLayoutConstraint activateConstraints:@[ On 2017/01/17 13:42:16, marq wrote: > On 2017/01/17 09:59:00, gambard wrote: > > On 2017/01/16 17:16:41, marq wrote: > > > This only works because _detailLabel and _interactionMenu are being removed > > from > > > their superview in -retract. If the implementation were changed to set the > > views > > > to hidden=YES instead of removing the view, each call of expand or retract > > would > > > generate new constraints. > > > > > > I think it would be a bit safer to keep arrays of additional constraints for > > > expanded and collapsed states and enable/disable them when toggling. > > > > > > Also the general procedure when adding and removing constraints is that it's > > > safe to remove (deactivate) constraints at any time, but it's best to add > new > > > constraints in -updateConstraints. > > > > Done. > > I might have missread > > > https://developer.apple.com/reference/uikit/uiview/1622512-updateconstraints?... > > but it states: > > "It is almost always cleaner and easier to update a constraint immediately > after > > the affecting change has occurred. For example, if you want to change a > > constraint in response to a button tap, make that change directly in the > > button’s action method." > > Looks like the guidelines have changed a bit, although they do still say to > centralize adding new constraints in -updateConstraints for best performance. > > Also 'updating' a constraint can just be changing a property (usually > |constant|) of an existing constraint. That's fine to do anywhere. > > I haven't had a chance to closely evaluate all the approaches to updating > constraints in complex views under iOS 9+, so I actually don't know the > advantages of keeping constraint activations in -updateConstraints. > > So I'll fall back on the general principle: use the approach that makes the code > cleanest, and then if you identify a performance issue, make adjustments. > > Which is another way of saying you can ignore the last paragraph of my earlier > comment ;-) Acknowledged. :) https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm (right): https://codereview.chromium.org/2625693002/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_view_controller.mm:94: if ([item isKindOfClass:[SuggestionsExpandableItem class]]) { On 2017/01/17 13:42:16, marq wrote: > On 2017/01/17 11:55:29, gambard wrote: > > On 2017/01/17 10:38:49, lpromero wrote: > > > This should just check the item type. > > > > As I am checking if it conforms to protocol, the itemType check is not needed > > for now. If I have different classes conforming to the protocol, I would use > the > > itemType to discriminate. > > Part of the benefit of the protocol is that you don't care what the class of the > conforming object is. Yes, I was meaning different objects conforming to the same protocol but potentially having different behavior. https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h (right): https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h:10: // Protocol allowing an item to be expanded. On 2017/01/17 13:42:16, marq wrote: > Specify 'CollectionViewItem' Done. https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_article.h:11: @protocol SuggestionsExpandableArticle On 2017/01/17 13:42:16, marq wrote: > Make it less specific: ExpandableItem. Done. https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm (right): https://codereview.chromium.org/2625693002/diff/180001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm:71: NSArray* _expandedConstraints; On 2017/01/17 13:42:16, marq wrote: > NSArray<NSLayoutConstraint*>* Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2625693002/#ps200001 (title: "Address 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": 200001, "attempt_start_ts": 1484664421406450,
"parent_rev": "e1cbbbc7b5abc4c741109257a0e673359590caf0", "commit_rev":
"9a26ea51a7b89d72ea18a9bf3583440a63be766d"}
Message was sent while issue was closed.
Description was changed from ========== Suggestions UI - expandable item BUG=679369 ========== to ========== Suggestions UI - expandable item BUG=679369 Review-Url: https://codereview.chromium.org/2625693002 Cr-Commit-Position: refs/heads/master@{#444056} Committed: https://chromium.googlesource.com/chromium/src/+/9a26ea51a7b89d72ea18a9bf3583... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/9a26ea51a7b89d72ea18a9bf3583...
Message was sent while issue was closed.
noyau@chromium.org changed reviewers: + noyau@chromium.org
Message was sent while issue was closed.
Sorry for the late comment. This was a draft and I forgot to send it. https://codereview.chromium.org/2625693002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h (right): https://codereview.chromium.org/2625693002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/suggestions/suggestions_collection_updater.h:18: (SuggestionsViewController*)collectionViewController Change the variable name as well for consistency? |
