chrome/browser/resources/settings/reset_page/reset_page_dialog.css - Issue 1482573002: MD Settings: Adjusting reset-page dialogs styling.

Side by Side Diff: chrome/browser/resources/settings/reset_page/reset_page_dialog.css

Issue 1482573002: MD Settings: Adjusting reset-page dialogs styling. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Tweaks Created 5 years ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« chrome/browser/resources/settings/reset_page/powerwash_dialog.html ('K') | « chrome/browser/resources/settings/reset_page/powerwash_dialog.html ('k') | chrome/browser/resources/settings/reset_page/reset_profile_dialog.css » ('j') | no next file with comments »
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 /* Copyright 2015 The Chromium Authors. All rights reserved.

	2 * Use of this source code is governed by a BSD-style license that can be

	3 * found in the LICENSE file. */

	4

	5 #dialog-content {

	6 -webkit-padding-end: 0;

	7 -webkit-padding-start: 0;

	8 margin-top: 0;

	9 }

	10

	11 .dialog-title {
	dschuyler 2015/12/02 01:02:01 Would .settings-box work here? Would .settings-box work here? dpapad 2015/12/02 03:07:49 I thought about this but it seems that settings-bo Show quoted text 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. Dan Beam 2015/12/02 03:26:43 agreed ^ we could compose out a "top-border" or " Show quoted text 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.
	12 border-bottom: 1px solid gainsboro;

	13 font-size: 1.13em;

	14 padding-bottom: 14px;

	15 padding-top: 14px;

	16 }

	17

	18 .dialog-body {

	19 font-size: 1em;

	20 margin: 20px 0;

	21 }

	22

	23 .dialog-title,

	24 .dialog-body {

	25 -webkit-padding-end: 24px;

	26 -webkit-padding-start: 24px;
	Dan Beam 2015/12/02 03:26:43 FYI: I don't disagree with what you're doing, but FYI: I don't disagree with what you're doing, but padding-left/right work fine when these are equal (i.e. directionality agnostic)
	27 }

	28

	29 .action-button {

	30 -webkit-margin-start: 10px;

	31 background-color: rgb(66, 133, 244);

	32 color: white;

	33 font-weight: 500;

	34 }

	35

	36 .cancel-button {
	dschuyler 2015/12/02 01:02:01 Let's change this to something like .highlight-but 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 I am not opposed to this, but I believe it is bett Show quoted text 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 fine with per-component css files, also fine with Show quoted text 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) dschuyler 2015/12/02 21:45:22 Nit: I feel that "cancel" is a bit over-specific - Show quoted text 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 I am a bit confused by the suggested naming (highl Show quoted text 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 why not just action-button or default-button for t Show quoted text 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...
	37 color: rgb(66, 133, 244);

	38 font-weight: 500;

	39 }

	40

	41 paper-button {

	42 margin: 0;

	43 }

	44

	45 .explanation {

	46 margin-bottom: 35px;

	47 }

	48

	49 .action-buttons {

	50 display: flex;

	51 flex: 1;

	52 justify-content: flex-end;

	53 }

OLD	NEW