Suggestions UI - expandable item

ios/chrome/browser/ui/suggestions/suggestions_expandable_item.mm

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#import "ios/chrome/browser/ui/suggestions/suggestions_expandable_item.h"
+
+#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#include "ios/chrome/grit/ios_strings.h"
+#include "ui/base/l10n/l10n_util_mac.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+namespace {
+const CGFloat kImageSize = 80;
+}
+
+#pragma mark - SuggestionsExpandableItem
+
+@implementation SuggestionsExpandableItem {
+  NSString* _title;
+  NSString* _subtitle;
+  UIImage* _image;
+  NSString* _detail;
+}
+
+@synthesize delegate = _delegate;
+@synthesize expanded = _expanded;
+
+- (instancetype)initWithType:(NSInteger)type
+                       title:(NSString*)title
+                    subtitle:(NSString*)subtitle
+                       image:(UIImage*)image
+                  detailText:(NSString*)detail {
+  self = [super initWithType:type];
+  if (self) {
+    self.cellClass = [SuggestionsExpandableCell class];
+    _title = [title copy];
+    _subtitle = [subtitle copy];
+    _image = image;
+    _detail = [detail copy];
+  }
+  return self;
+}
+
+#pragma mark - CollectionViewItem
+
+- (void)configureCell:(SuggestionsExpandableCell*)cell {
+  [super configureCell:cell];
+  cell.delegate = self.delegate;
+  cell.titleLabel.text = _title;
+  cell.subtitleLabel.text = _subtitle;
+  cell.imageView.image = _image;
+  cell.detailLabel.text = _detail;
+  if (self.expanded)
+    [cell expand];
+  else
+    [cell retract];
+}
+
+@end
+
+#pragma mark - SuggestionsExpandableCell
+
+@implementation SuggestionsExpandableCell {
+  UIView* _articleContainer;
+  UIButton* _interactionButton;
+  UIButton* _expandButton;
+  BOOL _isExpanded;

[marq (ping after 24h), 2017/01/16 17:16:41]

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];

[gambard, 2017/01/17 09:59:00]

Done.

+}
+
+@synthesize titleLabel = _titleLabel;
+@synthesize subtitleLabel = _subtitleLabel;
+@synthesize detailLabel = _detailLabel;
+@synthesize imageView = _imageView;
+@synthesize delegate = _delegate;
+
+- (instancetype)initWithFrame:(CGRect)frame {
+  self = [super initWithFrame:frame];
+  if (self) {
+    _isExpanded = NO;
+    _titleLabel = [[UILabel alloc] initWithFrame:CGRectZero];
+    _subtitleLabel = [[UILabel alloc] initWithFrame:CGRectZero];
+    _imageView = [[UIImageView alloc] initWithFrame:CGRectZero];
+    UIView* imageContainer = [[UIView alloc] initWithFrame:CGRectZero];
+    _articleContainer = [[UIView alloc] initWithFrame:CGRectZero];
+    _expandButton = [UIButton buttonWithType:UIButtonTypeSystem];
+    _detailLabel = [[UILabel alloc] initWithFrame:CGRectZero];
+    _interactionButton = [UIButton buttonWithType:UIButtonTypeSystem];
+
+    _subtitleLabel.numberOfLines = 0;
+    [_expandButton setTitle:@"See more" forState:UIControlStateNormal];
+    _detailLabel.numberOfLines = 0;
+    [_interactionButton setTitle:@"Less interaction"
+                        forState:UIControlStateNormal];
+
+    imageContainer.translatesAutoresizingMaskIntoConstraints = NO;
+    _imageView.translatesAutoresizingMaskIntoConstraints = NO;
+    _titleLabel.translatesAutoresizingMaskIntoConstraints = NO;
+    _subtitleLabel.translatesAutoresizingMaskIntoConstraints = NO;
+    _articleContainer.translatesAutoresizingMaskIntoConstraints = NO;
+    _expandButton.translatesAutoresizingMaskIntoConstraints = NO;
+    _detailLabel.translatesAutoresizingMaskIntoConstraints = NO;
+    _interactionButton.translatesAutoresizingMaskIntoConstraints = NO;
+
+    [_expandButton addTarget:self

[marq (ping after 24h), 2017/01/16 17:16:41]

Why not target:nil?

[gambard, 2017/01/17 09:59:00]

Because it does not work.

+                      action:@selector(expandPressed)

[marq (ping after 24h), 2017/01/16 17:16:41]

I prefer action methods to not depend on a particular UI representation. Ideally
these would just be expand: and retract: (or collapse:).

[gambard, 2017/01/17 09:59:00]

Done.

+            forControlEvents:UIControlEventTouchUpInside];
+    [_interactionButton addTarget:self
+                           action:@selector(retractPressed)
+                 forControlEvents:UIControlEventTouchUpInside];
+
+    [imageContainer addSubview:_imageView];

[marq (ping after 24h), 2017/01/16 17:16:41]

Can you explain in comments why the imageContainer is necessary?

[gambard, 2017/01/17 09:59:00]

Done.

+    [_articleContainer addSubview:imageContainer];
+    [_articleContainer addSubview:_titleLabel];
+    [_articleContainer addSubview:_subtitleLabel];
+
+    [self.contentView addSubview:_articleContainer];
+    [self.contentView addSubview:_expandButton];
+
+    [NSLayoutConstraint activateConstraints:@[
+      [self.contentView.centerXAnchor
+          constraintEqualToAnchor:_expandButton.centerXAnchor],
+      [_expandButton.topAnchor
+          constraintEqualToAnchor:_articleContainer.bottomAnchor],
+      [_expandButton.bottomAnchor
+          constraintEqualToAnchor:self.contentView.bottomAnchor]
+    ]];
+
+    ApplyVisualConstraintsWithMetrics(
+        @[
+          @"H:|[container]|", @"H:|-[title]-[imageContainer(imageSize)]-|",
+          @"H:|[image(imageSize)]", @"H:|-[text]-[imageContainer]",
+          @"V:|[image(imageSize)]", @"V:|-[title]-[text]-|",
+          @"V:|-[imageContainer(>=imageSize)]-|", @"V:|[container]"
+        ],
+        @{
+          @"image" : _imageView,
+          @"imageContainer" : imageContainer,
+          @"title" : _titleLabel,
+          @"text" : _subtitleLabel,
+          @"container" : _articleContainer
+        },
+        @{ @"imageSize" : @(kImageSize) });
+  }
+  return self;
+}
+
+#pragma mark - Private

[marq (ping after 24h), 2017/01/16 17:16:41]

Comments on all private methods.

[gambard, 2017/01/17 09:59:00]

Done.

+
+- (void)expandPressed {
+  [self.delegate expandCell:self];

[marq (ping after 24h), 2017/01/16 17:16:41]

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.

[gambard, 2017/01/17 11:55:29]

I don't like having to inspect all the cells to determine the one containing the
|sender|. I think this is better.

[marq (ping after 24h), 2017/01/17 13:42:16]

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.

[gambard, 2017/01/17 14:46:56]

Acknowledged.

+}
+
+- (void)retractPressed {
+  [self.delegate retractCell:self];
+}
+
+- (void)expand {
+  if (_isExpanded)
+    return;
+  _isExpanded = YES;
+
+  [self.contentView addSubview:_detailLabel];
+  [self.contentView addSubview:_interactionButton];
+  [_expandButton removeFromSuperview];
+
+  [NSLayoutConstraint activateConstraints:@[

[marq (ping after 24h), 2017/01/16 17:16:41]

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.

[gambard, 2017/01/17 09:59:00]

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."

[marq (ping after 24h), 2017/01/17 13:42:16]

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 ;-)

[gambard, 2017/01/17 14:46:56]

Acknowledged. :)

+    [_detailLabel.topAnchor
+        constraintEqualToAnchor:_articleContainer.bottomAnchor],
+    [_detailLabel.bottomAnchor
+        constraintEqualToAnchor:_interactionButton.topAnchor],
+    [_interactionButton.bottomAnchor
+        constraintEqualToAnchor:self.contentView.bottomAnchor],
+    [_detailLabel.leadingAnchor
+        constraintEqualToAnchor:self.contentView.leadingAnchor],
+    [_detailLabel.trailingAnchor
+        constraintEqualToAnchor:self.contentView.trailingAnchor]
+  ]];
+}
+
+- (void)retract {
+  if (!_isExpanded)
+    return;
+  _isExpanded = NO;
+
+  [_detailLabel removeFromSuperview];
+  [_interactionButton removeFromSuperview];
+  [self.contentView addSubview:_expandButton];
+
+  [NSLayoutConstraint activateConstraints:@[
+    [self.contentView.centerXAnchor
+        constraintEqualToAnchor:_expandButton.centerXAnchor],
+    [_expandButton.topAnchor
+        constraintEqualToAnchor:_articleContainer.bottomAnchor],
+    [_expandButton.bottomAnchor
+        constraintEqualToAnchor:self.contentView.bottomAnchor]
+  ]];
+}
+
+#pragma mark - UIView
+
+// Implements -layoutSubviews as per instructions in documentation for
+// +[MDCCollectionViewCell cr_preferredHeightForWidth:forItem:].
+- (void)layoutSubviews {
+  [super layoutSubviews];
+
+  // Adjust the text label preferredMaxLayoutWidth when the parent's width
+  // changes, for instance on screen rotation.
+  CGFloat parentWidth = CGRectGetWidth(self.contentView.bounds);
+  _subtitleLabel.preferredMaxLayoutWidth = parentWidth - kImageSize - 3 * 8;
+  _detailLabel.preferredMaxLayoutWidth = parentWidth;
+
+  // Re-layout with the new preferred width to allow the label to adjust its
+  // height.
+  [super layoutSubviews];
+}
+
+@end