Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2727603002:
Add first layout for the suggested articles (Closed)
|
Created:
3 years, 9 months ago by gambard Modified:
3 years, 9 months ago Reviewers:
Olivier CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd first layout for the suggested articles
This CL adds a layout to make the articles look better.
BUG=694220
Review-Url: https://codereview.chromium.org/2727603002
Cr-Commit-Position: refs/heads/master@{#453934}
Committed: https://chromium.googlesource.com/chromium/src/+/25eac092d62daca30149285866c0b70dfad089b4
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : Address comments #Patch Set 4 : Rebase #
Dependent Patchsets: Messages
Total messages: 17 (8 generated)
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
PTAL.
https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:77: - (void)applyConstraints; comment: Called from init. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:151: parentWidth - self.imageView.bounds.size.width - 3 * 8; what is 3 * 8 ? https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:164: [self.imageView.widthAnchor constraintEqualToConstant:kImageSize], I don't think you should use getter from init. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:165: [self.imageView.heightAnchor constraintEqualToConstant:kImageSize], nit: self.imageView.heightAnchor constraitEqualToAnchor:self.imageView.widthAnchor (easier to understand this is a square).
Thanks, PTAL. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:77: - (void)applyConstraints; On 2017/03/01 08:41:42, Olivier Robin wrote: > comment: Called from init. Done. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:151: parentWidth - self.imageView.bounds.size.width - 3 * 8; On 2017/03/01 08:41:42, Olivier Robin wrote: > what is 3 * 8 ? The margins. I have created a variable + a comment to make sure this is kept up to date. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:164: [self.imageView.widthAnchor constraintEqualToConstant:kImageSize], On 2017/03/01 08:41:43, Olivier Robin wrote: > I don't think you should use getter from init. Done. https://codereview.chromium.org/2727603002/diff/1/ios/chrome/browser/ui/conte... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:165: [self.imageView.heightAnchor constraintEqualToConstant:kImageSize], On 2017/03/01 08:41:42, Olivier Robin wrote: > nit: > self.imageView.heightAnchor constraitEqualToAnchor:self.imageView.widthAnchor > > (easier to understand this is a square). Done.
LGTM, 1 question. https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:78: // Applies the contraints on the elements. Called in the init. s/contraints/constraints/ https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:155: parentWidth - self.imageView.bounds.size.width - 3 * kStandardSpacing; question: why 3?
Thanks! https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm (right): https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:78: // Applies the contraints on the elements. Called in the init. On 2017/03/01 12:20:12, Olivier Robin wrote: > s/contraints/constraints/ Done. https://codereview.chromium.org/2727603002/diff/20001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:155: parentWidth - self.imageView.bounds.size.width - 3 * kStandardSpacing; On 2017/03/01 12:20:12, Olivier Robin wrote: > question: why 3? There are 3 margins: left border <-> Text <-> Image <-> right border.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2727603002/#ps40001 (title: "Address comments")
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 commit-bot@chromium.org
Failed to apply patch for
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:
While running git apply --index -p1;
error: patch failed:
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:58
error:
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm:
patch does not apply
Patch:
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
Index:
ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
diff --git
a/ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
b/ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
index
4749f4f7104e4e5a6e9e991f3c909bb4064cd77b..38e178bfb0e91e32ac5675538cc2f8c713977852
100644
---
a/ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
+++
b/ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.mm
@@ -5,14 +5,17 @@
#import
"ios/chrome/browser/ui/content_suggestions/content_suggestions_article_item.h"
#include "base/time/time.h"
+#import "ios/chrome/browser/ui/colors/MDCPalette+CrAdditions.h"
#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#import
"ios/third_party/material_components_ios/src/components/Typography/src/MaterialTypography.h"
#if !defined(__has_feature) || !__has_feature(objc_arc)
#error "This file requires ARC support."
#endif
namespace {
-const CGFloat kImageSize = 100;
+const CGFloat kImageSize = 72;
+// When updating this, make sure to update |layoutSubviews|.
const CGFloat kStandardSpacing = 8;
}
@@ -58,9 +61,9 @@ const CGFloat kStandardSpacing = 8;
if (!self.image && !self.imageBeingFetched) {
[self.delegate loadImageForArticleItem:self];
}
- cell.titleLabel.text = _title;
- cell.subtitleLabel.text = _subtitle;
- cell.imageView.image = _image;
+ cell.titleLabel.text = self.title;
+ cell.subtitleLabel.text = self.subtitle;
+ cell.imageView.image = self.image;
[cell setPublisherName:self.publisher date:self.publishDate];
}
@@ -72,6 +75,9 @@ const CGFloat kStandardSpacing = 8;
@property(nonatomic, strong) UILabel* publisherLabel;
+// Applies the constraints on the elements. Called in the init.
+- (void)applyConstraints;
+
@end
@implementation ContentSuggestionsArticleCell
@@ -89,12 +95,14 @@ const CGFloat kStandardSpacing = 8;
_imageView = [[UIImageView alloc] initWithFrame:CGRectZero];
_publisherLabel = [[UILabel alloc] initWithFrame:CGRectZero];
+ _titleLabel.numberOfLines = 2;
_subtitleLabel.numberOfLines = 0;
[_subtitleLabel setContentHuggingPriority:UILayoutPriorityDefaultHigh
forAxis:UILayoutConstraintAxisVertical];
[_titleLabel setContentHuggingPriority:UILayoutPriorityDefaultHigh
forAxis:UILayoutConstraintAxisVertical];
- _imageView.contentMode = UIViewContentModeScaleAspectFit;
+ _imageView.contentMode = UIViewContentModeScaleAspectFill;
+ _imageView.clipsToBounds = YES;
_imageView.translatesAutoresizingMaskIntoConstraints = NO;
_titleLabel.translatesAutoresizingMaskIntoConstraints = NO;
@@ -106,32 +114,14 @@ const CGFloat kStandardSpacing = 8;
[self.contentView addSubview:_subtitleLabel];
[self.contentView addSubview:_publisherLabel];
- [NSLayoutConstraint activateConstraints:@[
- [_imageView.widthAnchor constraintLessThanOrEqualToConstant:kImageSize],
- [_imageView.heightAnchor constraintLessThanOrEqualToConstant:kImageSize],
- [_publisherLabel.topAnchor
- constraintGreaterThanOrEqualToAnchor:_imageView.bottomAnchor
- constant:kStandardSpacing],
- [_publisherLabel.topAnchor
- constraintGreaterThanOrEqualToAnchor:_subtitleLabel.bottomAnchor
- constant:kStandardSpacing],
- ]];
-
- ApplyVisualConstraints(
- @[
- @"H:|-[title]-[image]-|",
- @"H:|-[text]-[image]",
- @"V:|-[title]-[text]",
- @"V:|-[image]",
- @"H:|-[publish]-|",
- @"V:[publish]-|",
- ],
- @{
- @"image" : _imageView,
- @"title" : _titleLabel,
- @"text" : _subtitleLabel,
- @"publish" : _publisherLabel,
- });
+ _titleLabel.font = [MDCTypography subheadFont];
+ _subtitleLabel.font = [MDCTypography body1Font];
+ _publisherLabel.font = [MDCTypography captionFont];
+
+ _subtitleLabel.textColor = [[MDCPalette greyPalette] tint700];
+ _publisherLabel.textColor = [[MDCPalette greyPalette] tint700];
+
+ [self applyConstraints];
}
return self;
}
@@ -158,12 +148,47 @@ const CGFloat kStandardSpacing = 8;
// Adjust the text label preferredMaxLayoutWidth when the parent's width
// changes, for instance on screen rotation.
CGFloat parentWidth = CGRectGetWidth(self.contentView.bounds);
+
+ self.titleLabel.preferredMaxLayoutWidth =
+ parentWidth - self.imageView.bounds.size.width - 3 * kStandardSpacing;
self.subtitleLabel.preferredMaxLayoutWidth =
- parentWidth - self.imageView.bounds.size.width - 3 * 8;
+ parentWidth - self.imageView.bounds.size.width - 3 * kStandardSpacing;
// Re-layout with the new preferred width to allow the label to adjust its
// height.
[super layoutSubviews];
}
+#pragma mark - Private
+
+- (void)applyConstraints {
+ [NSLayoutConstraint activateConstraints:@[
+ [_imageView.widthAnchor constraintEqualToConstant:kImageSize],
+ [_imageView.heightAnchor constraintEqualToAnchor:_imageView.widthAnchor],
+ [_publisherLabel.topAnchor
+ constraintGreaterThanOrEqualToAnchor:_imageView.bottomAnchor
+ constant:kStandardSpacing],
+ [_publisherLabel.topAnchor
+ constraintGreaterThanOrEqualToAnchor:_subtitleLabel.bottomAnchor
+ constant:kStandardSpacing],
+ ]];
+
+ ApplyVisualConstraintsWithMetrics(
+ @[
+ @"H:|-(space)-[title]-(space)-[image]-(space)-|",
+ @"H:|-(space)-[text]-(space)-[image]",
+ @"V:|-[title]-[text]",
+ @"V:|-[image]",
+ @"H:|-[publish]-|",
+ @"V:[publish]-|",
+ ],
+ @{
+ @"image" : _imageView,
+ @"title" : _titleLabel,
+ @"text" : _subtitleLabel,
+ @"publish" : _publisherLabel,
+ },
+ @{ @"space" : @(kStandardSpacing) });
+}
+
@end
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from olivierrobin@chromium.org Link to the patchset: https://codereview.chromium.org/2727603002/#ps60001 (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": 60001, "attempt_start_ts": 1488376740353000,
"parent_rev": "889af53fad72676272dd249f039a12aec8845f8c", "commit_rev":
"25eac092d62daca30149285866c0b70dfad089b4"}
Message was sent while issue was closed.
Description was changed from ========== Add first layout for the suggested articles This CL adds a layout to make the articles look better. BUG=694220 ========== to ========== Add first layout for the suggested articles This CL adds a layout to make the articles look better. BUG=694220 Review-Url: https://codereview.chromium.org/2727603002 Cr-Commit-Position: refs/heads/master@{#453934} Committed: https://chromium.googlesource.com/chromium/src/+/25eac092d62daca30149285866c0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/25eac092d62daca30149285866c0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
