MD Settings: Adding reset page Material UI dialog.

MD Settings: Adding reset page Material UI dialog.

BUG=546840
Committed: https://crrev.com/62eb5f1b0475f5adcf281fb42f2fa95335be6733
Cr-Commit-Position: refs/heads/master@{#357173}

[dpapad, 2015-10-28 23:40:57]

Patchset #2 (id:20001) has been deleted

[dpapad, 2015-10-28 23:42:41]

Description was changed from

==========
MD Settings: Reset page Material UI, WIP.

BUG=546840
==========

to

==========
MD Settings: Reset page Material UI, WIP.

BUG=546840
==========

[dpapad, 2015-10-28 23:42:55]

Description was changed from

==========
MD Settings: Reset page Material UI, WIP.

BUG=546840
==========

to

==========
MD Settings: Reset page Material UI.

BUG=546840
==========

[dpapad, 2015-10-28 23:45:35]

Patchset #2 (id:40001) has been deleted

[Dan Beam, 2015-10-29 00:46:10]

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

[Dan Beam, 2015-10-29 00:46:11]

can haz screenshots?

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:7: min-width: 300px;
i think internet settings also shows dialogs.  can we share some styles?

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:13: margin-left:
10px;
RTL

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:33: text-transform:
uppercase;
should we be using paper-button instead then?

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.html (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:23: target="_blank"
></a>
nit: remove \s after _blank"

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:25: <paper-button
on-tap="onHideDialog_" id="cancelButton"
nit: cancelButton -> cancel

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:25: <paper-button
on-tap="onHideDialog_" id="cancelButton"
onCancelTap_

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:27: <paper-button
i18n-content="resetPageCommit" on-tap="onReset_"
onResetTap_

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:32: <paper-checkbox
id="sendSettings" i18n-content="resetPageFeedback" checked></paper-checkbox>
wrap at 80 cols

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:24: properties: {},
not needed if empty

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:627:
AddResetStrings(html_source);
nit: in alpha order

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:636:
AddPrivacyStrings(html_source);
i.e. AddResetStrings() goes here

[dpapad, 2015-10-29 01:33:10]

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:7: min-width: 300px;
On 2015/10/29 00:46:10, Dan Beam wrote:
> i think internet settings also shows dialogs.  can we share some styles?

I 'll look into this tomorrow. Even if sharing CSS styles is possible, maybe
better to do it
on a separate CL though to keep this CL focused on adding the new dialog.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:13: margin-left:
10px;
On 2015/10/29 00:46:10, Dan Beam wrote:
> RTL

Are you suggesting using -webkit-margin-start instead? I had the impression
(don't remember how though), that these
are obsolete and left/right is properly reversed for RTL without the use of
-webkit-foo-{start|end}. Is that not so?

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:33: text-transform:
uppercase;
On 2015/10/29 00:46:10, Dan Beam wrote:
> should we be using paper-button instead then?

I tried this, but makes things more complicated. If I don't wrap the
paper-button with an extra div, then I
end up with a very long button that opens a new window even when user clicks
very far from the "Learn more" text.
--------------------------------------- ------
Learn more                  | Cancel  | Reset|
----------------------------------------------

The link does not have this issue (and does not require an extra div wrapper).
BTW we are already using both methods, at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
and
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
So I prefer to keep it as is.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.html (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:23: target="_blank"
></a>
On 2015/10/29 00:46:10, Dan Beam wrote:
> nit: remove \s after _blank"

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:25: <paper-button
on-tap="onHideDialog_" id="cancelButton"
On 2015/10/29 00:46:10, Dan Beam wrote:
> onCancelTap_

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:25: <paper-button
on-tap="onHideDialog_" id="cancelButton"
On 2015/10/29 00:46:10, Dan Beam wrote:
> nit: cancelButton -> cancel

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:27: <paper-button
i18n-content="resetPageCommit" on-tap="onReset_"
On 2015/10/29 00:46:10, Dan Beam wrote:
> onResetTap_

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.html:32: <paper-checkbox
id="sendSettings" i18n-content="resetPageFeedback" checked></paper-checkbox>
On 2015/10/29 00:46:10, Dan Beam wrote:
> wrap at 80 cols

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.js (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.js:24: properties: {},
On 2015/10/29 00:46:10, Dan Beam wrote:
> not needed if empty

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc
(right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:627:
AddResetStrings(html_source);
On 2015/10/29 00:46:11, Dan Beam wrote:
> nit: in alpha order

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/ui/webui...
chrome/browser/ui/webui/settings/md_settings_localized_strings_provider.cc:636:
AddPrivacyStrings(html_source);
On 2015/10/29 00:46:10, Dan Beam wrote:
> i.e. AddResetStrings() goes here

Done.

[Dan Beam, 2015-10-29 02:20:25]

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:13: margin-left:
10px;
On 2015/10/29 01:33:09, dpapad wrote:
> On 2015/10/29 00:46:10, Dan Beam wrote:
> > RTL
> 
> Are you suggesting using -webkit-margin-start instead?

yes

> I had the impression
> (don't remember how though), that these
> are obsolete and left/right is properly reversed for RTL without the use of
> -webkit-foo-{start|end}. Is that not so?

not so

margin-left: http://fiddle.jshell.net/8nn6t5sp/1/
-webkit-margin-start: http://fiddle.jshell.net/8nn6t5sp/2/

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:33: text-transform:
uppercase;
On 2015/10/29 01:33:09, dpapad wrote:
> On 2015/10/29 00:46:10, Dan Beam wrote:
> > should we be using paper-button instead then?
> 
> I tried this, but makes things more complicated. If I don't wrap the
> paper-button with an extra div, then I
> end up with a very long button that opens a new window even when user clicks
> very far from the "Learn more" text.
> --------------------------------------- ------
> Learn more                  | Cancel  | Reset|
> ----------------------------------------------
> 
> The link does not have this issue (and does not require an extra div wrapper).
> BTW we are already using both methods, at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> and
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
> So I prefer to keep it as is.

an <a> tag with text-transform uppercase is very very different than a
<paper-button>.  what about the ripple?  what about focus behavior?  keyboard
shortcuts?  we should act the same way if we're gonna attempt to look the same.

here's what I think you should do: http://jsfiddle.net/o0r0azk9/

[dpapad, 2015-10-29 17:59:19]

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:13: margin-left:
10px;
On 2015/10/29 02:20:25, Dan Beam wrote:
> On 2015/10/29 01:33:09, dpapad wrote:
> > On 2015/10/29 00:46:10, Dan Beam wrote:
> > > RTL
> > 
> > Are you suggesting using -webkit-margin-start instead?
> 
> yes
> 
> > I had the impression
> > (don't remember how though), that these
> > are obsolete and left/right is properly reversed for RTL without the use of
> > -webkit-foo-{start|end}. Is that not so?
> 
> not so
> 
> margin-left: http://fiddle.jshell.net/8nn6t5sp/1/
> -webkit-margin-start: http://fiddle.jshell.net/8nn6t5sp/2/

Done.

https://codereview.chromium.org/1418073008/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:33: text-transform:
uppercase;
On 2015/10/29 02:20:25, Dan Beam wrote:
> On 2015/10/29 01:33:09, dpapad wrote:
> > On 2015/10/29 00:46:10, Dan Beam wrote:
> > > should we be using paper-button instead then?
> > 
> > I tried this, but makes things more complicated. If I don't wrap the
> > paper-button with an extra div, then I
> > end up with a very long button that opens a new window even when user clicks
> > very far from the "Learn more" text.
> > --------------------------------------- ------
> > Learn more                  | Cancel  | Reset|
> > ----------------------------------------------
> > 
> > The link does not have this issue (and does not require an extra div
wrapper).
> > BTW we are already using both methods, at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res...
> > and
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/res....
> > So I prefer to keep it as is.
> 
> an <a> tag with text-transform uppercase is very very different than a
> <paper-button>.  what about the ripple?  what about focus behavior?  keyboard
> shortcuts?  we should act the same way if we're gonna attempt to look the
same.
> 
> here's what I think you should do: http://jsfiddle.net/o0r0azk9/

Done.

[dpapad, 2015-10-29 18:52:24]

Description was changed from

==========
MD Settings: Reset page Material UI.

BUG=546840
==========

to

==========
MD Settings: Adding reset page Material UI dialog.

BUG=546840
==========

[Dan Beam, 2015-10-29 22:02:00]

lgtm

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:20: }
^ why is this necessary?

[dpapad, 2015-10-30 17:46:00]

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:20: }
On 2015/10/29 22:02:00, Dan Beam wrote:
> ^ why is this necessary?

This achieves aligning the text "LEARN MORE" with "CANCEL" and "RESET".
The latter two, are within a container div that has extra padding on the top,
where as
"LEARN MORE" paper-button resides outside cancel's and reset's container. Using
margin:auto
seems the most elegant way of aligning them.

[dpapad, 2015-10-30 17:58:58]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2015-10-30 18:00:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418073008/60002
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418073008/60002

[Dan Beam, 2015-10-30 18:11:27]

The CQ bit was unchecked by dbeam@chromium.org

[dpapad, 2015-10-30 18:30:24]

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page.css (right):

https://codereview.chromium.org/1418073008/diff/60002/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page.css:20: }
On 2015/10/30 17:45:59, dpapad wrote:
> On 2015/10/29 22:02:00, Dan Beam wrote:
> > ^ why is this necessary?
> 
> This achieves aligning the text "LEARN MORE" with "CANCEL" and "RESET".
> The latter two, are within a container div that has extra padding on the top,
> where as
> "LEARN MORE" paper-button resides outside cancel's and reset's container.
Using
> margin:auto
> seems the most elegant way of aligning them.

Removed as discussed, and using "layout center" on the container.

[dpapad, 2015-10-30 18:32:06]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2015-10-30 18:32:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/1418073008/#ps150001
(title: "use layout center, remove margin auto")

[commit-bot: I haz the power, 2015-10-30 18:35:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1418073008/150001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1418073008/150001

[commit-bot: I haz the power, 2015-10-30 19:35:26]

Committed patchset #7 (id:150001)

[commit-bot: I haz the power, 2015-10-30 19:36:09]

Patchset 7 (id:??) landed as
https://crrev.com/62eb5f1b0475f5adcf281fb42f2fa95335be6733
Cr-Commit-Position: refs/heads/master@{#357173}