MD Settings: Adjusting reset-page dialogs styling.

MD Settings: Adjusting reset-page dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

BUG=546840
Committed: https://crrev.com/23e1aae3bda7cab5146673b299c62fa28bf54532
Cr-Commit-Position: refs/heads/master@{#363044}

[dpapad, 2015-11-26 01:10:27]

Description was changed from

==========
MD Settings: Adjusting reset profile and powerwash styling.

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

to

==========
MD Settings: Adjusting dialogs styling.

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

[dpapad, 2015-12-01 22:47:07]

Patchset #2 (id:20001) has been deleted

[dpapad, 2015-12-02 00:05:33]

Description was changed from

==========
MD Settings: Adjusting dialogs styling.

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

to

==========
MD Settings: Adjusting dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

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

[dpapad, 2015-12-02 00:21:11]

dpapad@chromium.org changed reviewers:
+ dschuyler@chromium.org

[dpapad, 2015-12-02 00:21:12]

Before and after screenshots at http://imgur.com/a/bwCLf.

These changes bring the dialogs much closer to the mocks, but not 100%, because
I am still waiting for some clarifications.

[dpapad, 2015-12-02 00:21:35]

Description was changed from

==========
MD Settings: Adjusting dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

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

to

==========
MD Settings: Adjusting reset-page dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

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

[dschuyler, 2015-12-02 01:02:01]

I'm hoping we can reduce separation of css definitions.  Can some of these be
added to settings_page.css to make styling more widely available (and used)?

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:11:
.dialog-title {
Would .settings-box work here?

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
Let's change this to something like .highlight-button or .default-button and
move it into settings_page.css.  It would be cool to have this set across all MD
settings pages consistently.

[dpapad, 2015-12-02 03:07:49]

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:11:
.dialog-title {
On 2015/12/02 at 01:02:01, dschuyler wrote:
> Would .settings-box work here?

I thought about this but it seems that settings-box and dialog-title share very
little. Basically they only share the fact that there is a border (either top or
bottom), but everything else is different (margin/padding/min-height). Moreover,
having a settings-box class within a modal dialog seemed more confusing than
it's worth.

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
On 2015/12/02 at 01:02:01, dschuyler wrote:
> Let's change this to something like .highlight-button or .default-button and
move it into settings_page.css.  It would be cool to have this set across all MD
settings pages consistently.

I am not opposed to this, but I believe it is better done as a CSS refactoring
when we have concrete examples of buttons across different UIs that we know
should be styled the same. Looked through the mocks and I do see some candidates
(People-Cros section has some "Done" buttons), but those sections have not been
implemented yet. Also when that time comes, we might want to have greater
granularity by having something like checkbox.css, button.css, dropdown.css,
instead of dumping things to settings_page.css.

[Dan Beam, 2015-12-02 03:26:43]

excuse the drive-by

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:11:
.dialog-title {
On 2015/12/02 03:07:49, dpapad wrote:
> On 2015/12/02 at 01:02:01, dschuyler wrote:
> > Would .settings-box work here?
> 
> I thought about this but it seems that settings-box and dialog-title share
very
> little. Basically they only share the fact that there is a border (either top
or
> bottom), but everything else is different (margin/padding/min-height).
Moreover,
> having a settings-box class within a modal dialog seemed more confusing than
> it's worth.

agreed ^

we could compose out a "top-border" or "separator" class for the common
functionality, but I don't see a point.  if we really care, we could use a
--css-var for the color or border, but this may have performance issues.

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:26:
-webkit-padding-start: 24px;
FYI: I don't disagree with what you're doing, but padding-left/right work fine
when these are equal (i.e. directionality agnostic)

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
On 2015/12/02 03:07:49, dpapad wrote:
> On 2015/12/02 at 01:02:01, dschuyler wrote:
> > Let's change this to something like .highlight-button or .default-button and
> move it into settings_page.css.  It would be cool to have this set across all
MD
> settings pages consistently.
> 
> I am not opposed to this, but I believe it is better done as a CSS refactoring
> when we have concrete examples of buttons across different UIs that we know
> should be styled the same. Looked through the mocks and I do see some
candidates
> (People-Cros section has some "Done" buttons), but those sections have not
been
> implemented yet. Also when that time comes, we might want to have greater
> granularity by having something like checkbox.css, button.css, dropdown.css,
> instead of dumping things to settings_page.css.

fine with per-component css files, also fine with deferring until meta-patterns
emerge (2+ users of code)

[dpapad, 2015-12-02 21:34:06]

Friendly ping. Anything left to address here?

[dschuyler, 2015-12-02 21:45:22]

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/powerwash_dialog.html (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/powerwash_dialog.html:17: <div
class="layout center horizontal">
The style guide suggests not using inlined layout center horizontal.
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
On 2015/12/02 03:07:49, dpapad wrote:
> On 2015/12/02 at 01:02:01, dschuyler wrote:
> > Let's change this to something like .highlight-button or .default-button and
> move it into settings_page.css.  It would be cool to have this set across all
MD
> settings pages consistently.
> 
> I am not opposed to this, but I believe it is better done as a CSS refactoring
> when we have concrete examples of buttons across different UIs that we know
> should be styled the same. Looked through the mocks and I do see some
candidates
> (People-Cros section has some "Done" buttons), but those sections have not
been
> implemented yet. Also when that time comes, we might want to have greater
> granularity by having something like checkbox.css, button.css, dropdown.css,
> instead of dumping things to settings_page.css.

Nit: I feel that "cancel" is a bit over-specific -- even if it's kept in this
file.

[dpapad, 2015-12-02 22:21:59]

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/powerwash_dialog.html (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/powerwash_dialog.html:17: <div
class="layout center horizontal">
On 2015/12/02 at 21:45:22, dschuyler wrote:
> The style guide suggests not using inlined layout center horizontal.
>
https://docs.google.com/document/d/1G0t519y_cSW4etXVwruR145abZH0N5-9R7JyejzVo...

Done.

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
On 2015/12/02 at 21:45:22, dschuyler wrote:
> On 2015/12/02 03:07:49, dpapad wrote:
> > On 2015/12/02 at 01:02:01, dschuyler wrote:
> > > Let's change this to something like .highlight-button or .default-button
and
> > move it into settings_page.css.  It would be cool to have this set across
all MD
> > settings pages consistently.
> > 
> > I am not opposed to this, but I believe it is better done as a CSS
refactoring
> > when we have concrete examples of buttons across different UIs that we know
> > should be styled the same. Looked through the mocks and I do see some
candidates
> > (People-Cros section has some "Done" buttons), but those sections have not
been
> > implemented yet. Also when that time comes, we might want to have greater
> > granularity by having something like checkbox.css, button.css, dropdown.css,
> > instead of dumping things to settings_page.css.
> 
> Nit: I feel that "cancel" is a bit over-specific -- even if it's kept in this
file.

I am a bit confused by the suggested naming (highlight/default button). The
"cancel" button is not
highlighted or default in any way. I have named the highlighted button (the one
having blue background)
as "action-button" to make it generic, and it is the "reset" in the reset dialog
and "restart" in the
powerwash dialog.

The cancel-button class always refers to a button with text "Cancel" in both
dialogs where it is currently used.
Also the dialogs which will reuse this CSS file very soon are the ones at
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...,
which both have a "Cancel" button. Can you suggest a name that you think it
would be better? other-button?

[Dan Beam, 2015-12-02 23:12:09]

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/reset_page/reset_page_dialog.css (right):

https://codereview.chromium.org/1482573002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/reset_page/reset_page_dialog.css:36:
.cancel-button {
On 2015/12/02 22:21:59, dpapad wrote:
> On 2015/12/02 at 21:45:22, dschuyler wrote:
> > On 2015/12/02 03:07:49, dpapad wrote:
> > > On 2015/12/02 at 01:02:01, dschuyler wrote:
> > > > Let's change this to something like .highlight-button or .default-button
> and
> > > move it into settings_page.css.  It would be cool to have this set across
> all MD
> > > settings pages consistently.
> > > 
> > > I am not opposed to this, but I believe it is better done as a CSS
> refactoring
> > > when we have concrete examples of buttons across different UIs that we
know
> > > should be styled the same. Looked through the mocks and I do see some
> candidates
> > > (People-Cros section has some "Done" buttons), but those sections have not
> been
> > > implemented yet. Also when that time comes, we might want to have greater
> > > granularity by having something like checkbox.css, button.css,
dropdown.css,
> > > instead of dumping things to settings_page.css.
> > 
> > Nit: I feel that "cancel" is a bit over-specific -- even if it's kept in
this
> file.
> 
> I am a bit confused by the suggested naming (highlight/default button). The
> "cancel" button is not
> highlighted or default in any way. I have named the highlighted button (the
one
> having blue background)
> as "action-button" to make it generic, and it is the "reset" in the reset
dialog
> and "restart" in the
> powerwash dialog.
> 
> The cancel-button class always refers to a button with text "Cancel" in both
> dialogs where it is currently used.
> Also the dialogs which will reuse this CSS file very soon are the ones at
>
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Inner%20Pag...,
> which both have a "Cancel" button. Can you suggest a name that you think it
> would be better? other-button?

why not just action-button or default-button for the one that we want users to
click?  (note: some dialogs wont have a default)

also, I know webui hasn't been a great example to follow considering our tech
debt, but this particular argument could probably just be solved by following an
existing example:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...

[dschuyler, 2015-12-02 23:19:51]

I'll look into this further and check on non-cancel buttons in dialogs.
For now, that doesn't need to hold up this CL.  lgtm

[dpapad, 2015-12-02 23:45:57]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2015-12-02 23:48:12]

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

[commit-bot: I haz the power, 2015-12-02 23:54:39]

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

[commit-bot: I haz the power, 2015-12-02 23:54:40]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[dpapad, 2015-12-03 00:55:25]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2015-12-03 00:57:45]

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

[commit-bot: I haz the power, 2015-12-03 01:03:46]

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

[commit-bot: I haz the power, 2015-12-03 01:03:47]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2015-12-03 17:46:55]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2015-12-03 17:46:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1482573002/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-03 17:47:12]

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

[commit-bot: I haz the power, 2015-12-03 17:50:20]

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

[commit-bot: I haz the power, 2015-12-03 17:50:21]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2015-12-03 18:34:16]

Patchset #6 (id:120001) has been deleted

[dpapad, 2015-12-03 18:49:57]

Patchset #7 (id:160001) has been deleted

[dpapad, 2015-12-03 18:50:04]

Patchset #6 (id:140001) has been deleted

[dpapad, 2015-12-03 18:50:11]

Patchset #5 (id:100001) has been deleted

[dpapad, 2015-12-03 18:50:28]

Patchset #4 (id:80001) has been deleted

[dpapad, 2015-12-03 18:55:50]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2015-12-03 18:55:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/1482573002/#ps180001
(title: "rebase")

[commit-bot: I haz the power, 2015-12-03 18:57:25]

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

[commit-bot: I haz the power, 2015-12-03 20:22:48]

Committed patchset #4 (id:180001)

[commit-bot: I haz the power, 2015-12-03 20:24:21]

Description was changed from

==========
MD Settings: Adjusting reset-page dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

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

to

==========
MD Settings: Adjusting reset-page dialogs styling to match latest mocks.

Refactor both dialogs such that CSS styles can be shared
between them (see reset_page_dialog.css).

BUG=546840

Committed: https://crrev.com/23e1aae3bda7cab5146673b299c62fa28bf54532
Cr-Commit-Position: refs/heads/master@{#363044}
==========

[commit-bot: I haz the power, 2015-12-03 20:24:22]

Patchset 4 (id:??) landed as
https://crrev.com/23e1aae3bda7cab5146673b299c62fa28bf54532
Cr-Commit-Position: refs/heads/master@{#363044}