New issue
Advanced search Search tips

Issue 591585 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 547073



Sign in to add a comment

MD Settings: Refactor dialogs, re-use base <settings-dialog> element.

Project Member Reported by dpa...@chromium.org, Mar 3 2016

Issue description

Currently we are duplicating code across most of our dialogs, and share styling by carefully selecting CSS class/id names that exist in settings_dialog_css.html (for  example #dialog-content, .body .explanation, .title).

As part of implementing the "Certificate manager" section, I need to add four more dialogs, so I think it might be worth creating a new <settings-dialog> element, and re-use it from existing dialogs (and from the dialogs I am about to add). I am expecting the final result to look as follows

<my-new-dialog>
  <settings-dialog>
    <div class='title'>Title goes here</div>
    <div class='body'>Dialog body goes here</div>
    <div class='button-container'>buttons go here</div>
  <settings-dialog>
</my-new-dialog>

The new <settings-dialog> element would take care of styling/positioning title, body and button-container appropriately without duplicating code (which is our current approach), by using the <content> tag with a filter, for example 
<content select=".class-name-here"></content>
 
Blocking: 547073
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c671afb138abe2e0c7e2bc69df1fb9da9e224c89

commit c671afb138abe2e0c7e2bc69df1fb9da9e224c89
Author: dpapad <dpapad@chromium.org>
Date: Fri Mar 04 21:50:48 2016

MD Settings: Creating a <settings-dialog> element.

The new element uses the Light DOM to get populated. Example
<my-new-dialog>
  <settings-dialog>
    <div class='title'>Title goes here</div>
    <div class='body'>Dialog body goes here</div>
    <div class='button-container'>buttons go here</div>
    <div class='footer'>footer content goes here</div>
  <settings-dialog>
</my-new-dialog>

Using the new dialog achieves code re-use for
 1) Positioning title, body and button-container and footer.
 2) Providing an 'x' close button at the top right.

BUG= 591585 

Review URL: https://codereview.chromium.org/1758973002

Cr-Commit-Position: refs/heads/master@{#379369}

[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/on_startup_page/startup_urls_page.html
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/reset_page/powerwash_dialog.html
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/reset_page/reset_page_dialog.css
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/reset_page/reset_profile_dialog.css
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/reset_page/reset_profile_dialog.html
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/reset_page/reset_profile_dialog.js
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/search_engines_page/search_engine_dialog.html
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/search_engines_page/search_engine_dialog.js
[add] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/settings_dialog.html
[add] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/settings_dialog.js
[delete] https://crrev.com/1fb948d6a6b3e06e43fbd6bc8b5b772969797752/chrome/browser/resources/settings/settings_dialog_css.html
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/browser/resources/settings/settings_resources.grd
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/test/data/webui/settings/reset_page_test.js
[modify] https://crrev.com/c671afb138abe2e0c7e2bc69df1fb9da9e224c89/chrome/test/data/webui/settings/search_engines_page_test.js

Status: Fixed (was: Started)

Sign in to add a comment