Adding close button in SigninPromoView

Adding close button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

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

[jlebel, 2017-04-28 10:59:59]

Patchset #2 (id:20001) has been deleted

[jlebel, 2017-04-28 11:00:08]

Patchset #1 (id:1) has been deleted

[jlebel, 2017-04-28 11:16:33]

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

[jlebel, 2017-04-28 11:16:34]

Hello Mihai and Louis,

Can you review this patch to add the dismiss button for the sign-in promo in the
bookmarks?

Thanks,

[jlebel, 2017-04-28 11:41:32]

Patchset #1 (id:40001) has been deleted

[jlebel, 2017-04-28 11:42:48]

Description was changed from

==========
Adding dismiss button in SigninPromoView

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

to

==========
Adding dismiss button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

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

[msarda, 2017-04-28 16:08:16]

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/BUILD.gn (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/BUILD.gn:12:
"resources/signin_promo_dismiss_gray.png",
This is a general comment that applies to all your CL:
I prefer "close" to "dismiss" for the name of this button. That would mean
changing "dismiss" to "close" in all your CL, so it would be a good idea to get
a second opinion (ideally from a native english speaker) before making this
change.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/signin_promo_view.h (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/signin_promo_view.h:39:
@property(nonatomic, readonly) UIButton* dismissButton;
Maybe add a comment saying that this button is hidden by default.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:36:
_signinPromoView.dismissButton.hidden = NO;
It looks like the button is only used by the BookmarksPromoCell. Could it be
moved from the SigninPromoView to BookmarkSigninPromoCell (basically make it a
subview of the BookmarkSigninPromoCell's contentView)?

That would allow us to add the SignInPromoView to other UIs in the future that
may have a different dismiss button.

[jlebel, 2017-04-28 16:22:57]

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

[jlebel, 2017-04-28 16:22:58]

Hello Peter,

Do you have any suggestion name about the X button to remove Sign-in promo?
https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

Is this button a close button? Or is this button a dismiss button? (or something
else...)

Thanks,

[pkl (ping after 24h if needed), 2017-04-30 06:09:35]

On 2017/04/28 16:22:58, jlebel wrote:
> Hello Peter,
> 
> Do you have any suggestion name about the X button to remove Sign-in promo?
> https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
> https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k
> 
> Is this button a close button? Or is this button a dismiss button? (or
something
> else...)
> 
> Thanks,

Need permissions to view the Drive docs.

[jlebel, 2017-04-30 08:01:45]

On 2017/04/30 06:09:35, pkl -pls.ping.after.24h. wrote:
> On 2017/04/28 16:22:58, jlebel wrote:
> > Hello Peter,
> > 
> > Do you have any suggestion name about the X button to remove Sign-in promo?
> > https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
> > https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k
> > 
> > Is this button a close button? Or is this button a dismiss button? (or
> something
> > else...)
> > 
> > Thanks,
> 
> Need permissions to view the Drive docs.

It should work for you now. Thanks,

[pkl (ping after 24h if needed), 2017-04-30 08:10:27]

On 2017/04/30 08:01:45, jlebel wrote:
> On 2017/04/30 06:09:35, pkl -pls.ping.after.24h. wrote:
> > On 2017/04/28 16:22:58, jlebel wrote:
> > > Hello Peter,
> > > 
> > > Do you have any suggestion name about the X button to remove Sign-in
promo?
> > > https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
> > > https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k
> > > 
> > > Is this button a close button? Or is this button a dismiss button? (or
> > something
> > > else...)
> > > 
> > > Thanks,
> > 
> > Need permissions to view the Drive docs.
> 
> It should work for you now. Thanks,

I'm starting to feel that you may be looking for pschaffner@ for input on this X
button.
IMHO, the "X" button concept is a bit strange for a mobile/touch UI, but you may
not be asking me for input :)

[jlebel, 2017-04-30 08:14:47]

On 2017/04/30 08:10:27, pkl -pls.ping.after.24h. wrote:
> On 2017/04/30 08:01:45, jlebel wrote:
> > On 2017/04/30 06:09:35, pkl -pls.ping.after.24h. wrote:
> > > On 2017/04/28 16:22:58, jlebel wrote:
> > > > Hello Peter,
> > > > 
> > > > Do you have any suggestion name about the X button to remove Sign-in
> promo?
> > > > https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
> > > > https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k
> > > > 
> > > > Is this button a close button? Or is this button a dismiss button? (or
> > > something
> > > > else...)
> > > > 
> > > > Thanks,
> > > 
> > > Need permissions to view the Drive docs.
> > 
> > It should work for you now. Thanks,
> 
> I'm starting to feel that you may be looking for pschaffner@ for input on this
X
> button.
> IMHO, the "X" button concept is a bit strange for a mobile/touch UI, but you
may
> not be asking me for input :)

That's what I've been ask to implement:
https://drive.google.com/corp/drive/u/0/folders/0B3pahfDhxF6yTXNSY0JQaVNIb2M
But now that you mention it, I think you are right... I will see with Mihai and
Peter. Thanks,

[lpromero, 2017-05-02 16:46:05]

lgtm

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/BUILD.gn (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/BUILD.gn:12:
"resources/signin_promo_dismiss_gray.png",
On 2017/04/28 16:08:16, msarda wrote:
> This is a general comment that applies to all your CL:
> I prefer "close" to "dismiss" for the name of this button. That would mean
> changing "dismiss" to "close" in all your CL, so it would be a good idea to
get
> a second opinion (ideally from a native english speaker) before making this
> change.

I agree it would fit better with the naming we already have in the app.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:418:
addTarget:self
I don't think it's an issue in your case, but conceptually, if the cell was
reused, would you be added twice (and thus called twice) when someone presses
the button? Should you remove yourself as a target at some point, or in
BookmarkSigninPromoCell's prepareForReuse, should you remove all targets?

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:36:
_signinPromoView.dismissButton.hidden = NO;
On 2017/04/28 16:08:16, msarda wrote:
> It looks like the button is only used by the BookmarksPromoCell. Could it be
> moved from the SigninPromoView to BookmarkSigninPromoCell (basically make it a
> subview of the BookmarkSigninPromoCell's contentView)?
> 
> That would allow us to add the SignInPromoView to other UIs in the future that
> may have a different dismiss button.

Good idea.

[pkl (ping after 24h if needed), 2017-05-02 22:45:30]

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

[jlebel, 2017-05-03 12:41:14]

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/BUILD.gn (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/BUILD.gn:12:
"resources/signin_promo_dismiss_gray.png",
On 2017/05/02 16:46:04, lpromero wrote:
> On 2017/04/28 16:08:16, msarda wrote:
> > This is a general comment that applies to all your CL:
> > I prefer "close" to "dismiss" for the name of this button. That would mean
> > changing "dismiss" to "close" in all your CL, so it would be a good idea to
> get
> > a second opinion (ideally from a native english speaker) before making this
> > change.
> 
> I agree it would fit better with the naming we already have in the app.

Done.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
File ios/chrome/browser/ui/authentication/signin_promo_view.h (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/a...
ios/chrome/browser/ui/authentication/signin_promo_view.h:39:
@property(nonatomic, readonly) UIButton* dismissButton;
On 2017/04/28 16:08:16, msarda wrote:
> Maybe add a comment saying that this button is hidden by default.

Done.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_folder_collection_view.mm:418:
addTarget:self
On 2017/05/02 16:46:04, lpromero wrote:
> I don't think it's an issue in your case, but conceptually, if the cell was
> reused, would you be added twice (and thus called twice) when someone presses
> the button? Should you remove yourself as a target at some point, or in
> BookmarkSigninPromoCell's prepareForReuse, should you remove all targets?

Done.

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2844253003/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:36:
_signinPromoView.dismissButton.hidden = NO;
On 2017/05/02 16:46:04, lpromero wrote:
> On 2017/04/28 16:08:16, msarda wrote:
> > It looks like the button is only used by the BookmarksPromoCell. Could it be
> > moved from the SigninPromoView to BookmarkSigninPromoCell (basically make it
a
> > subview of the BookmarkSigninPromoCell's contentView)?
> > 
> > That would allow us to add the SignInPromoView to other UIs in the future
that
> > may have a different dismiss button.
> 
> Good idea.

I would hope for the user, we will use the same kind of mechanism to dismiss
this promo...

[jlebel, 2017-05-03 13:17:29]

Description was changed from

==========
Adding dismiss button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

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

to

==========
Adding close button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

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

[msarda, 2017-05-04 11:48:08]

Note that I think there was an error while uploading the icons in this CL, as
when I try to check the is leads to error "Upload of
ios/chrome/browser/ui/authentication/resources/signin_promo_close_gray.png in
progress. [FetchError]"

Please re-upload the CL before landing it.

Otherwise, LGTM

https://codereview.chromium.org/2844253003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2844253003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:85:
_closeButtonAction();
I think it would be better to set the closeButtonAction to nil after it is
called once. This would avoid it being called multiple times if the user taps
quickly more than once on the close button.

[jlebel, 2017-05-04 13:20:38]

https://codereview.chromium.org/2844253003/diff/100001/ios/chrome/browser/ui/...
File ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm (right):

https://codereview.chromium.org/2844253003/diff/100001/ios/chrome/browser/ui/...
ios/chrome/browser/ui/bookmarks/bookmark_signin_promo_cell.mm:85:
_closeButtonAction();
On 2017/05/04 11:48:08, msarda wrote:
> I think it would be better to set the closeButtonAction to nil after it is
> called once. This would avoid it being called multiple times if the user taps
> quickly more than once on the close button.

I don't believe it is relevant. This should be done where the action is done.

[jlebel, 2017-05-04 13:20:42]

The CQ bit was checked by jlebel@chromium.org

[jlebel, 2017-05-04 13:20:43]

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/2844253003/#ps100001
(title: "Changing dismiss to close, and moving the button to the cell")

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

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-05-04 15:03:17]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493904042901310,
"parent_rev": "643d6bc90fead3ccefa9309d265dd048e51c263f", "commit_rev":
"b5da0c5b0047718b2957621c55b436ae25a87c45"}

[commit-bot: I haz the power, 2017-05-04 15:03:30]

Description was changed from

==========
Adding close button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

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

to

==========
Adding close button in SigninPromoView

https://drive.google.com/open?id=0ByXziH_JVCGJXzVLNmx6cTVtczA
https://drive.google.com/open?id=0ByXziH_JVCGJQ3BKS2JpOXA5a1k

BUG=661794

Review-Url: https://codereview.chromium.org/2844253003
Cr-Commit-Position: refs/heads/master@{#469333}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5da0c5b0047718b2957621c55b4...
==========

[commit-bot: I haz the power, 2017-05-04 15:03:32]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b5da0c5b0047718b2957621c55b4...