CollectionViewTextItem no longer styles CollectionViewTextCell

CollectionViewTextItem should not style CollectionViewTextCell

This is essentially a revert of https://codereview.chromium.org/2627323003
with the addition of adding styling to the view controllers using the 
CollectionViewTextCell.

This helps keep the styling logic flat and more flexible for the heavily reused CollectionViewTextItem.

[Moe, 2017-04-13 16:11:11]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 16:11:39]

Dry run: 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-04-13 16:15:10]

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

[commit-bot: I haz the power, 2017-04-13 16:15:11]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Moe, 2017-04-13 16:16:00]

Description was changed from

==========
CollectionViewTextItem no longer styles CollectionViewTextCell
==========

to

==========
CollectionViewTextItem should not style CollectionViewTextCell

This is essentially a revert of https://codereview.chromium.org/2627323003
with the addition of adding styling to the view controllers using the 
CollectionViewTextCell.

This helps keep the styling logic flat and more flexible for the heavily reused
CollectionViewTextItem.
==========

[Moe, 2017-04-13 16:16:00]

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

[Moe, 2017-04-13 16:16:25]

Hi Louis, Gauthier, PTAL

[Moe, 2017-04-13 16:32:45]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 16:33:30]

Dry run: 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-04-13 16:42:56]

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

[commit-bot: I haz the power, 2017-04-13 16:42:57]

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

[Moe, 2017-04-13 16:52:18]

Patchset #2 (id:20001) has been deleted

[Moe, 2017-04-13 16:52:30]

The CQ bit was checked by mahmadi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 16:52:49]

Dry run: 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-04-13 17:04:19]

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

[commit-bot: I haz the power, 2017-04-13 17:04:19]

Dry run: This issue passed the CQ dry run.

[lpromero, 2017-04-13 17:44:26]

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm
(left):

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm:39:
_textFont = [[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
Instead of moving these defaults in all the View Controllers, I would have moved
them to the CollectionViewTextCell. Then VCs can override when needed.

[Moe, 2017-04-13 18:48:58]

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm
(left):

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm:39:
_textFont = [[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
On 2017/04/13 17:44:26, lpromero wrote:
> Instead of moving these defaults in all the View Controllers, I would have
moved
> them to the CollectionViewTextCell. Then VCs can override when needed.

If the VCs only override the properties, don't we risk inadvertently changing
the font/color in a lot of places if the defaults in the cell change for some
reason?
I think having no defaults in the cell should force the author of the VC to
fully style it when using it and guard again those changes. wdyt?

[Moe, 2017-04-13 19:33:39]

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm
(left):

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm:39:
_textFont = [[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
On 2017/04/13 17:44:26, lpromero wrote:
> Instead of moving these defaults in all the View Controllers, I would have
moved
> them to the CollectionViewTextCell. Then VCs can override when needed.

Done.

[lpromero, 2017-04-13 20:24:55]

The CQ bit was checked by lpromero@chromium.org

[lpromero, 2017-04-13 20:24:55]

lgtm

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm
(left):

https://codereview.chromium.org/2817953002/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/collection_view/cells/collection_view_text_item.mm:39:
_textFont = [[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
On 2017/04/13 18:48:58, moe wrote:
> On 2017/04/13 17:44:26, lpromero wrote:
> > Instead of moving these defaults in all the View Controllers, I would have
> moved
> > them to the CollectionViewTextCell. Then VCs can override when needed.
> 
> If the VCs only override the properties, don't we risk inadvertently changing
> the font/color in a lot of places if the defaults in the cell change for some
> reason?
> I think having no defaults in the cell should force the author of the VC to
> fully style it when using it and guard again those changes. wdyt?

It's sort of the point of having the default be like a "style". If specs change,
we change only in one place. But since on the other end, we sometimes override
these values, we are neither fully explicit nor DRY :(

[commit-bot: I haz the power, 2017-04-13 20:25:32]

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-04-13 20:29:39]

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

[commit-bot: I haz the power, 2017-04-13 20:29:40]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Moe, 2017-04-13 20:52:21]

On 2017/04/13 20:29:40, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
>   mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Thank you.

[Moe, 2017-04-13 20:52:26]

The CQ bit was checked by mahmadi@chromium.org

[Moe, 2017-04-13 20:52:26]

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/2817953002/#ps70018
(title: "rebase")

[commit-bot: I haz the power, 2017-04-13 20:52:57]

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-04-13 21:03:58]

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

[commit-bot: I haz the power, 2017-04-13 21:04:00]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[gambard, 2017-04-14 07:04:03]

I am not fond of this change, it is also a revert of
https://codereview.chromium.org/2768933005/
I had to make the above CL because I changed the cell class used by the
CollectionViewItem. As you are making a cast in all those classes, changing the
cell means changing all those casts.

Is there any reason for doing this change?

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm
(right):

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:378:
cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
Is this working?
I am surprised, it is a header not a cell. You should probably implement
- (UICollectionReusableView*)collectionView:(UICollectionView*)collectionView	
           viewForSupplementaryElementOfKind:(NSString*)kind	
                                 atIndexPath:(NSIndexPath*)indexPath

This is also true for the other header through the CL.

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
(right):

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:815:
textCell.textLabel.textColor = [[MDCPalette greyPalette] tint500];
Didn't you move from red to grey?

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
File
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
(left):

https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:186:
item.textColor = [[MDCPalette cr_redPalette] tint500];
This one is red

[lpromero, 2017-04-14 07:23:24]

On 2017/04/14 07:04:03, gambard wrote:
> I am not fond of this change, it is also a revert of
> https://codereview.chromium.org/2768933005/
> I had to make the above CL because I changed the cell class used by the
> CollectionViewItem. As you are making a cast in all those classes, changing
the
> cell means changing all those casts.
> 
> Is there any reason for doing this change?
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm
> (right):
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:378:
> cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
> Is this working?
> I am surprised, it is a header not a cell. You should probably implement
> - (UICollectionReusableView*)collectionView:(UICollectionView*)collectionView	
>            viewForSupplementaryElementOfKind:(NSString*)kind	
>                                  atIndexPath:(NSIndexPath*)indexPath
> 
> This is also true for the other header through the CL.
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> File
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
> (right):
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:815:
> textCell.textLabel.textColor = [[MDCPalette greyPalette] tint500];
> Didn't you move from red to grey?
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> File
> ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
> (left):
> 
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
>
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:186:
> item.textColor = [[MDCPalette cr_redPalette] tint500];
> This one is red

Maybe it's time to start designing and implementing the styler then :|

[lpromero, 2017-04-14 11:09:09]

On 2017/04/14 07:23:24, lpromero wrote:
> On 2017/04/14 07:04:03, gambard wrote:
> > I am not fond of this change, it is also a revert of
> > https://codereview.chromium.org/2768933005/
> > I had to make the above CL because I changed the cell class used by the
> > CollectionViewItem. As you are making a cast in all those classes, changing
> the
> > cell means changing all those casts.
> > 
> > Is there any reason for doing this change?
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm
> > (right):
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:378:
> > cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
> > Is this working?
> > I am surprised, it is a header not a cell. You should probably implement
> > -
(UICollectionReusableView*)collectionView:(UICollectionView*)collectionView	
> >            viewForSupplementaryElementOfKind:(NSString*)kind	
> >                                  atIndexPath:(NSIndexPath*)indexPath
> > 
> > This is also true for the other header through the CL.
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > File
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
> > (right):
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:815:
> > textCell.textLabel.textColor = [[MDCPalette greyPalette] tint500];
> > Didn't you move from red to grey?
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > File
> >
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
> > (left):
> > 
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> >
>
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:186:
> > item.textColor = [[MDCPalette cr_redPalette] tint500];
> > This one is red
> 
> Maybe it's time to start designing and implementing the styler then :|

Moe, Gauthier and I talked about a styler API. Gauthier is doing a prototype CL
and I start writing a doc. Are you block by any of this? I am fine keeping the
code as is if you are not blocked.

[Moe, 2017-04-15 00:35:58]

On 2017/04/14 11:09:09, lpromero wrote:
> On 2017/04/14 07:23:24, lpromero wrote:
> > On 2017/04/14 07:04:03, gambard wrote:
> > > I am not fond of this change, it is also a revert of
> > > https://codereview.chromium.org/2768933005/
> > > I had to make the above CL because I changed the cell class used by the
> > > CollectionViewItem. As you are making a cast in all those classes,
changing
> > the
> > > cell means changing all those casts.
> > > 
> > > Is there any reason for doing this change?
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > File ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:378:
> > > cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
> > > Is this working?
> > > I am surprised, it is a header not a cell. You should probably implement
> > > -
> (UICollectionReusableView*)collectionView:(UICollectionView*)collectionView	
> > >            viewForSupplementaryElementOfKind:(NSString*)kind	
> > >                                  atIndexPath:(NSIndexPath*)indexPath
> > > 
> > > This is also true for the other header through the CL.
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > File
> > >
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > >
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:815:
> > > textCell.textLabel.textColor = [[MDCPalette greyPalette] tint500];
> > > Didn't you move from red to grey?
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > File
> > >
> ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
> > > (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > >
> >
>
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:186:
> > > item.textColor = [[MDCPalette cr_redPalette] tint500];
> > > This one is red
> > 
> > Maybe it's time to start designing and implementing the styler then :|
> 
> Moe, Gauthier and I talked about a styler API. Gauthier is doing a prototype
CL
> and I start writing a doc. Are you block by any of this? I am fine keeping the
> code as is if you are not blocked.

I saw the style API. and I like it! I think it's a step in the right direction.
Thank you for the prototype Gaughier. It is very useful.
I am blocked by CollectionViewTextItem's implementation as is. But I'm going to
make changes to PaymentsTextCell to
support a secondary text label and replace all instances of
CollectionViewTextItem with PaymentsTextItem.
That will unblock me. Maybe I can give the styler a try early on too :)

[Moe, 2017-04-15 00:36:12]

On 2017/04/15 00:35:58, moe wrote:
> On 2017/04/14 11:09:09, lpromero wrote:
> > On 2017/04/14 07:23:24, lpromero wrote:
> > > On 2017/04/14 07:04:03, gambard wrote:
> > > > I am not fond of this change, it is also a revert of
> > > > https://codereview.chromium.org/2768933005/
> > > > I had to make the above CL because I changed the cell class used by the
> > > > CollectionViewItem. As you are making a cast in all those classes,
> changing
> > > the
> > > > cell means changing all those casts.
> > > > 
> > > > Is there any reason for doing this change?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > > File
ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > >
ios/chrome/browser/ui/settings/accounts_collection_view_controller.mm:378:
> > > > cellForItemAtIndexPath:(nonnull NSIndexPath*)indexPath {
> > > > Is this working?
> > > > I am surprised, it is a header not a cell. You should probably implement
> > > > -
> > (UICollectionReusableView*)collectionView:(UICollectionView*)collectionView	
> > > >            viewForSupplementaryElementOfKind:(NSString*)kind	
> > > >                                  atIndexPath:(NSIndexPath*)indexPath
> > > > 
> > > > This is also true for the other header through the CL.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > > File
> > > >
> > >
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > >
> > >
> >
>
ios/chrome/browser/ui/settings/clear_browsing_data_collection_view_controller.mm:815:
> > > > textCell.textLabel.textColor = [[MDCPalette greyPalette] tint500];
> > > > Didn't you move from red to grey?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > > File
> > > >
> >
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm
> > > > (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2817953002/diff/70018/ios/chrome/browser/ui/s...
> > > >
> > >
> >
>
ios/chrome/browser/ui/settings/password_details_collection_view_controller.mm:186:
> > > > item.textColor = [[MDCPalette cr_redPalette] tint500];
> > > > This one is red
> > > 
> > > Maybe it's time to start designing and implementing the styler then :|
> > 
> > Moe, Gauthier and I talked about a styler API. Gauthier is doing a prototype
> CL
> > and I start writing a doc. Are you block by any of this? I am fine keeping
the
> > code as is if you are not blocked.
> 
> I saw the style API. and I like it! I think it's a step in the right
direction.
> Thank you for the prototype Gaughier. It is very useful.
> I am blocked by CollectionViewTextItem's implementation as is. But I'm going
to
> make changes to PaymentsTextCell to
> support a secondary text label and replace all instances of
> CollectionViewTextItem with PaymentsTextItem.
> That will unblock me. Maybe I can give the styler a try early on too :)

Closing this.