Create CollectionViewTextCell

Create CollectionViewTextCell

This CL creates a CollectionViewTextCell having the same behavior as
MDCCollectionViewTextCell but using auto layout.

BUG=704082
Review-Url: https://codereview.chromium.org/2764783006
Cr-Commit-Position: refs/heads/master@{#459395}
Committed: https://chromium.googlesource.com/chromium/src/+/90a41e827e84da99b9746f6b9a355aa9e83ab6cd

[gambard, 2017-03-22 13:43:46]

gambard@chromium.org changed reviewers:
+ lpromero@chromium.org

[gambard, 2017-03-22 13:43:47]

PTAL.

[lpromero, 2017-03-22 15:51:47]

Nice!

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:39:
constraintGreaterThanOrEqualToConstant:kMinimalHeight],
Why? Material spec?

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:42:
[containerView.leadingAnchor
Why do you need the container? I'd remove it.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:55:
constraintEqualToAnchor:self.contentView.centerYAnchor],
You already set top and bottom, why do you need this one?

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:61:
constraintLessThanOrEqualToAnchor:containerView.trailingAnchor],
Why not going all the way to the trailing edge?

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:90: }
Can you add prepareForReuse?

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:91: 
Can you add proper accessibility? I think it's just a matter of setting
isAccessibilityElement to self (check other cells), and implementing
accessibilityLabel to concatenate the two labels strings.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:126:
@"Text cell with text that is so long it must truncate at some point";
This should not truncate anymore, right?

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:133:
@"truncate at some point.";
Why does it truncate? The point is to never truncate.

[gambard, 2017-03-23 09:41:30]

Thanks, PTAL.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:39:
constraintGreaterThanOrEqualToConstant:kMinimalHeight],
On 2017/03/22 15:51:47, lpromero wrote:
> Why? Material spec?

Yes.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:42:
[containerView.leadingAnchor
On 2017/03/22 15:51:47, lpromero wrote:
> Why do you need the container? I'd remove it.

The container is needed to center the labels in case the font is too small and
the sum margins+fontSize is smaller than the minimal height.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:55:
constraintEqualToAnchor:self.contentView.centerYAnchor],
On 2017/03/22 15:51:47, lpromero wrote:
> You already set top and bottom, why do you need this one?

Same as above.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:61:
constraintLessThanOrEqualToAnchor:containerView.trailingAnchor],
On 2017/03/22 15:51:47, lpromero wrote:
> Why not going all the way to the trailing edge?

We had some problem with RTL + editing when the label is taking all the width.
See crbug.com/698379

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:90: }
On 2017/03/22 15:51:47, lpromero wrote:
> Can you add prepareForReuse?

Done.
I am not sure about what to reset in prepareForReuse. I cannot reset everything
in the labels.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:91: 
On 2017/03/22 15:51:47, lpromero wrote:
> Can you add proper accessibility? I think it's just a matter of setting
> isAccessibilityElement to self (check other cells), and implementing
> accessibilityLabel to concatenate the two labels strings.

The accessibility is done in the item.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:126:
@"Text cell with text that is so long it must truncate at some point";
On 2017/03/22 15:51:47, lpromero wrote:
> This should not truncate anymore, right?

It is truncating because it is the current behavior. Maybe some implementation
depends on this.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:133:
@"truncate at some point.";
On 2017/03/22 15:51:47, lpromero wrote:
> Why does it truncate? The point is to never truncate.

I have added an option in the item to change the number of line.

[lpromero, 2017-03-23 12:23:36]

lgtm

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:55:
constraintEqualToAnchor:self.contentView.centerYAnchor],
On 2017/03/23 09:41:29, gambard wrote:
> On 2017/03/22 15:51:47, lpromero wrote:
> > You already set top and bottom, why do you need this one?
> 
> Same as above.

I see. And I didn't notice that the top/bottom where inequalities. So it's not
over-constrained.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:82:
CGFloat prefferedMaxLayoutWidth =
preferred

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:90: }
On 2017/03/23 09:41:29, gambard wrote:
> On 2017/03/22 15:51:47, lpromero wrote:
> > Can you add prepareForReuse?
> 
> Done.
> I am not sure about what to reset in prepareForReuse. I cannot reset
everything
> in the labels.

I guess we'll have to do incrementally.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:91: 
On 2017/03/23 09:41:29, gambard wrote:
> On 2017/03/22 15:51:47, lpromero wrote:
> > Can you add proper accessibility? I think it's just a matter of setting
> > isAccessibilityElement to self (check other cells), and implementing
> > accessibilityLabel to concatenate the two labels strings.
> 
> The accessibility is done in the item.

Acknowledged.

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:94:
self.textLabel.text = @"";
Use nil

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:95:
self.detailTextLabel.text = @"";
My rule of thumb is to reset everything set in configureCell:. But it's not
always easy, as in this example where they are not even in the same file.
Fine as is.

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h:33: //
The maximum number of lines of the main text. The Default value is 1.
default

Or: Default is 1.

[gambard, 2017-03-23 15:32:29]

Thanks!

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:82:
CGFloat prefferedMaxLayoutWidth =
On 2017/03/23 12:23:35, lpromero wrote:
> preferred

Done.

https://codereview.chromium.org/2764783006/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:90: }
On 2017/03/23 12:23:35, lpromero wrote:
> On 2017/03/23 09:41:29, gambard wrote:
> > On 2017/03/22 15:51:47, lpromero wrote:
> > > Can you add prepareForReuse?
> > 
> > Done.
> > I am not sure about what to reset in prepareForReuse. I cannot reset
> everything
> > in the labels.
> 
> I guess we'll have to do incrementally.

Acknowledged.

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:94:
self.textLabel.text = @"";
On 2017/03/23 12:23:35, lpromero wrote:
> Use nil

Done.

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_cell.mm:95:
self.detailTextLabel.text = @"";
On 2017/03/23 12:23:35, lpromero wrote:
> My rule of thumb is to reset everything set in configureCell:. But it's not
> always easy, as in this example where they are not even in the same file.
> Fine as is.

Acknowledged.

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h:33: //
The maximum number of lines of the main text. The Default value is 1.
On 2017/03/23 12:23:35, lpromero wrote:
> default
> 
> Or: Default is 1.

Done.
It is not consistent with the other comments :)

[gambard, 2017-03-23 15:32:32]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-23 15:32:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2764783006/#ps60001
(title: "Address comments")

[commit-bot: I haz the power, 2017-03-23 15:32:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 16:41:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 16:41:04]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[lpromero, 2017-03-23 19:16:33]

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h:33: //
The maximum number of lines of the main text. The Default value is 1.
On 2017/03/23 15:32:28, gambard wrote:
> On 2017/03/23 12:23:35, lpromero wrote:
> > default
> > 
> > Or: Default is 1.
> 
> Done.
> It is not consistent with the other comments :)

True. What happened to those?? Can you make it consistent by changing the wrong
ones?

[gambard, 2017-03-24 08:54:05]

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h
(right):

https://codereview.chromium.org/2764783006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.h:33: //
The maximum number of lines of the main text. The Default value is 1.
On 2017/03/23 19:16:33, lpromero wrote:
> On 2017/03/23 15:32:28, gambard wrote:
> > On 2017/03/23 12:23:35, lpromero wrote:
> > > default
> > > 
> > > Or: Default is 1.
> > 
> > Done.
> > It is not consistent with the other comments :)
> 
> True. What happened to those?? Can you make it consistent by changing the
wrong
> ones?

Done.

[gambard, 2017-03-24 10:01:35]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-03-24 10:01:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2764783006/#ps80001
(title: "Fix tests")

[commit-bot: I haz the power, 2017-03-24 10:02:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 11:59:04]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490349695118850,
"parent_rev": "d77e079e9b0d9849016be08bdcf39461f16f5230", "commit_rev":
"90a41e827e84da99b9746f6b9a355aa9e83ab6cd"}

[commit-bot: I haz the power, 2017-03-24 11:59:55]

Description was changed from

==========
Create CollectionViewTextCell

This CL creates a CollectionViewTextCell having the same behavior as
MDCCollectionViewTextCell but using auto layout.

BUG=704082
==========

to

==========
Create CollectionViewTextCell

This CL creates a CollectionViewTextCell having the same behavior as
MDCCollectionViewTextCell but using auto layout.

BUG=704082

Review-Url: https://codereview.chromium.org/2764783006
Cr-Commit-Position: refs/heads/master@{#459395}
Committed:
https://chromium.googlesource.com/chromium/src/+/90a41e827e84da99b9746f6b9a35...
==========

[commit-bot: I haz the power, 2017-03-24 11:59:56]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/90a41e827e84da99b9746f6b9a35...