iOS: Adding cell for the promo signin for settings.

iOS: Adding cell for the promo signin for settings.

Adding SignPromoItem as a collection view item and SigninPromoCell as a
MDC collection view cell. The cell is added into
MaterialCellCatalogViewController to be able to test.

https://drive.google.com/file/d/0ByXziH_JVCGJQzU1WlZ6bEVIT1E/view
https://drive.google.com/file/d/0ByXziH_JVCGJZC1NMmNfWnF1MG8/view
https://drive.google.com/file/d/0ByXziH_JVCGJVDRYdGhtUTFWU2M/view

BUG=661794
Review-Url: https://codereview.chromium.org/2743633002
Cr-Commit-Position: refs/heads/master@{#456446}
Committed: https://chromium.googlesource.com/chromium/src/+/9ebaa50c508af10810bafc3f06757269ebd63a93

[jlebel, 2017-03-09 10:20:49]

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

[jlebel, 2017-03-09 10:20:49]

Hello Louis,

Can you review my patch? Thanks.

I have 2 issues:
signin_promo_item.mm:98 IsCompact(self) doesn't work.

I'm not sure what to do with methods for Accessibility in signin_promo_item.mm.

Jérôme,

[lpromero, 2017-03-09 10:55:03]

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_chromium_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
personal

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
(s)he or it or they?

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_google_chrome_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_google_chrome_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
Idem

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_google_chrome_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
(s)he or it?

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1517: Not <ph
name="EMAIL">$1<ex>john.doe@gmail.com</ex>?</ph>
s/gmail.com/example.com

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/BUILD.gn (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/BUILD.gn:32: "signin_promo_item.mm",
Please add a simple unittest file.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/signin_promo_item.h (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

Prefer using ./tools/boilerplate.py
ios/chrome/browser/ui/settings/cells/signin_promo_item.h next time :)

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:11: #import
"ios/third_party/material_components_ios/src/components/Buttons/src/MaterialButtons.h"
Not needed in the header.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:14: // SigninPromoItem
is an Item that displays an Image, a title text label and a
Remove "is an Item that". Actually, the item never displays anything. Consider
moving the description to the cell adn just have the comment for the item be:
Item for SigninPromoCell.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:16: // This is intended
to be used as an sign in Item which contains a default
a sign-in item

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:17: // avatar and
information
Unwrap.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:19: // Item will allow
them
Unwrap.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:19: // Item will allow
them
They tap on the button, not the cell, no?

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/signin_promo_item.mm (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:25: // Vertical
paddingn for buttons.
padding

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:43:
self.accessibilityTraits |= UIAccessibilityTraitButton;
I wouldn't care for accessibility within the item, directly within the cell.
The item can carry accessibility traits from its clients to the cell, but here,
the cell can manage that directly.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:61: NSString*
_profileName;
Doesn't seem needed to keep it in an ivar.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:98: if
(IsCompact(self)) {
You mean it returns NO here? Probably because when first called, the cell is not
displayed and might be regular by default. Isn't traitsCollectionDidChange below
called when the view is added to the hierarchy?

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:134: _textLabel.font =
[[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
Please use MDCTypography instead.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:230: #pragma mark -
NSObject(Accessibility)
Let's discuss accessibility possibilities IRL.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm
(right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:409:
return YES;
Wow good catch!

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:471:
signinPromoItem.profileEmail = @"jane@gmail.com";
example.com

[jlebel, 2017-03-09 20:34:08]

Thanks for your input.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_chromium_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/09 10:55:02, lpromero wrote:
> personal

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/09 10:55:02, lpromero wrote:
> (s)he or it or they?

does (s)he work?

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_google_chrome_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_google_chrome_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/09 10:55:02, lpromero wrote:
> Idem

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_google_chrome_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/09 10:55:02, lpromero wrote:
> (s)he or it?

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_strings.grd:1517: Not <ph
name="EMAIL">$1<ex>john.doe@gmail.com</ex>?</ph>
On 2017/03/09 10:55:02, lpromero wrote:
> http://s/gmail.com/example.com

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/BUILD.gn (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/BUILD.gn:32: "signin_promo_item.mm",
On 2017/03/09 10:55:02, lpromero wrote:
> Please add a simple unittest file.

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/signin_promo_item.h (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/03/09 10:55:02, lpromero wrote:
> 2017
> 
> Prefer using ./tools/boilerplate.py
> ios/chrome/browser/ui/settings/cells/signin_promo_item.h next time :)

Acknowledged.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:11: #import
"ios/third_party/material_components_ios/src/components/Buttons/src/MaterialButtons.h"
On 2017/03/09 10:55:02, lpromero wrote:
> Not needed in the header.

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:17: // avatar and
information
On 2017/03/09 10:55:02, lpromero wrote:
> Unwrap.

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.h:19: // Item will allow
them
On 2017/03/09 10:55:02, lpromero wrote:
> They tap on the button, not the cell, no?

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/signin_promo_item.mm (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/03/09 10:55:03, lpromero wrote:
> 2017

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:25: // Vertical
paddingn for buttons.
On 2017/03/09 10:55:03, lpromero wrote:
> padding

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:61: NSString*
_profileName;
On 2017/03/09 10:55:02, lpromero wrote:
> Doesn't seem needed to keep it in an ivar.

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:98: if
(IsCompact(self)) {
On 2017/03/09 10:55:03, lpromero wrote:
> You mean it returns NO here? Probably because when first called, the cell is
not
> displayed and might be regular by default. Isn't traitsCollectionDidChange
below
> called when the view is added to the hierarchy?

Done.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm
(right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:409:
return YES;
On 2017/03/09 10:55:03, lpromero wrote:
> Wow good catch!

Acknowledged.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/material_cell_catalog_view_controller.mm:471:
signinPromoItem.profileEmail = @"jane@gmail.com";
On 2017/03/09 10:55:03, lpromero wrote:
> http://example.com

Done.

[pkl (ping after 24h if needed), 2017-03-10 00:32:34]

pkl@chromium.org changed reviewers:
+ pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-10 00:32:34]

drive-by

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_chromium_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/09 20:34:07, jlebel wrote:
> On 2017/03/09 10:55:02, lpromero wrote:
> > personal
> 
> Done.

Sorry to jump in here. The "singular they" is the least awkward way of phrasing
this, i.e. "..inform the user that they can sign in..."

[jlebel, 2017-03-10 09:13:26]

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
File ios/chrome/app/strings/ios_chromium_strings.grd (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/app/strings/ios_...
ios/chrome/app/strings/ios_chromium_strings.grd:291: <message
name="IDS_IOS_SIGNIN_PROMO_SETTINGS" desc="Text to inform the user that he can
sign in to get personnal settings">
On 2017/03/10 00:32:34, pklpkl wrote:
> On 2017/03/09 20:34:07, jlebel wrote:
> > On 2017/03/09 10:55:02, lpromero wrote:
> > > personal
> > 
> > Done.
> 
> Sorry to jump in here. The "singular they" is the least awkward way of
phrasing
> this, i.e. "..inform the user that they can sign in..."

Don't be sorry. That's interesting! A good friend of mine told me about singular
they, but I never found an opportunity to use it. I guess now is the right time!
Thanks,

[jlebel, 2017-03-13 16:00:42]

lpromero ping :)

[lpromero, 2017-03-13 16:05:45]

lgtm.

What is the current status for accessibility?

[jlebel, 2017-03-13 17:08:42]

I guess there is not better to do since I can't catch the default action.

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
File ios/chrome/browser/ui/settings/cells/signin_promo_item.mm (right):

https://codereview.chromium.org/2743633002/diff/1/ios/chrome/browser/ui/setti...
ios/chrome/browser/ui/settings/cells/signin_promo_item.mm:134: _textLabel.font =
[[MDFRobotoFontLoader sharedInstance] mediumFontOfSize:14];
On 2017/03/09 10:55:03, lpromero wrote:
> Please use MDCTypography instead.

Done.

[jlebel, 2017-03-13 17:16:21]

The CQ bit was checked by jlebel@chromium.org

[commit-bot: I haz the power, 2017-03-13 17:17:00]

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-13 18:46:01]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489425381321800,
"parent_rev": "05107990b575defc12ba29da8ae17a8660a29b67", "commit_rev":
"9ebaa50c508af10810bafc3f06757269ebd63a93"}

[commit-bot: I haz the power, 2017-03-13 18:46:51]

Description was changed from

==========
iOS: Adding cell for the promo signin for settings.

Adding SignPromoItem as a collection view item and SigninPromoCell as a
MDC collection view cell. The cell is added into
MaterialCellCatalogViewController to be able to test.

https://drive.google.com/file/d/0ByXziH_JVCGJQzU1WlZ6bEVIT1E/view
https://drive.google.com/file/d/0ByXziH_JVCGJZC1NMmNfWnF1MG8/view
https://drive.google.com/file/d/0ByXziH_JVCGJVDRYdGhtUTFWU2M/view

BUG=661794
==========

to

==========
iOS: Adding cell for the promo signin for settings.

Adding SignPromoItem as a collection view item and SigninPromoCell as a
MDC collection view cell. The cell is added into
MaterialCellCatalogViewController to be able to test.

https://drive.google.com/file/d/0ByXziH_JVCGJQzU1WlZ6bEVIT1E/view
https://drive.google.com/file/d/0ByXziH_JVCGJZC1NMmNfWnF1MG8/view
https://drive.google.com/file/d/0ByXziH_JVCGJVDRYdGhtUTFWU2M/view

BUG=661794

Review-Url: https://codereview.chromium.org/2743633002
Cr-Commit-Position: refs/heads/master@{#456446}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ebaa50c508af10810bafc3f0675...
==========

[commit-bot: I haz the power, 2017-03-13 18:46:52]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/9ebaa50c508af10810bafc3f0675...