chrome/browser/resources/settings/about_page/about_page.html - Issue 1950423003: MD Settings: Adding "About" page basic structure.

Side by Side Diff: chrome/browser/resources/settings/about_page/about_page.html

Issue 1950423003: MD Settings: Adding "About" page basic structure. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@about_page_route

Patch Set: Add GYP target. Created 4 years, 7 months 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

OLD	NEW
(Empty)
	1 <link rel="import" href="chrome://resources/html/polymer.html">

	2 <link rel="import" href="chrome://md-settings/settings_page/settings_section.htm l">

	3 <link rel="import" href="chrome://md-settings/settings_shared_css.html">

	4 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-butt on.html">
	tommycli 2016/05/06 18:01:43 Do you also need to import main_page_behavior? Do you also need to import main_page_behavior? dpapad 2016/05/06 18:33:29 I looked into this. MainPageBehavior has methods r Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > Do you also need to import main_page_behavior? I looked into this. MainPageBehavior has methods related to opening/closing subsections. Given that the About page does not have any subsections (unlike 'basic' and 'abvanced') it is not needed. tommycli 2016/05/06 18:50:20 I asked because you use main-page-styles below. If Show quoted text On 2016/05/06 18:33:29, dpapad wrote: > On 2016/05/06 at 18:01:43, tommycli wrote: > > Do you also need to import main_page_behavior? > > I looked into this. MainPageBehavior has methods related to opening/closing > subsections. Given that the About page does not have any subsections (unlike > 'basic' and 'abvanced') it is not needed. I asked because you use main-page-styles below. If it's unneeded, you could just fold the custom styles into the settings-shared block? dpapad 2016/05/06 19:01:31 Done. Show quoted text On 2016/05/06 at 18:50:20, tommycli wrote: > On 2016/05/06 18:33:29, dpapad wrote: > > On 2016/05/06 at 18:01:43, tommycli wrote: > > > Do you also need to import main_page_behavior? > > > > I looked into this. MainPageBehavior has methods related to opening/closing > > subsections. Given that the About page does not have any subsections (unlike > > 'basic' and 'abvanced') it is not needed. > > I asked because you use main-page-styles below. If it's unneeded, you could just fold the custom styles into the settings-shared block? Done.
	5

	6 <dom-module id="settings-about-page">

	7 <link rel="import" type="css" href="chrome://md-settings/settings_page.css">

	8 <template>

	9 <style include="settings-shared"></style>

	10 <style include="main-page-styles">

	11 .logo-container {
	tommycli 2016/05/06 18:01:43 Which is the element with the logo-container class Which is the element with the logo-container class? dpapad 2016/05/06 18:33:29 Removed. That was a remainder from a previous stat Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > Which is the element with the logo-container class? Removed. That was a remainder from a previous state of my code, where I was not using settings-box yet.
	12 display: flex;

	13 }
	tommycli 2016/05/06 18:01:43 \n between entries in css \n between entries in css dpapad 2016/05/06 18:33:29 Done. Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > \n between entries in css Done.
	14 span {

	15 font-size: 20px;

	16 margin-top: auto;

	17 margin-bottom: auto;
	tommycli 2016/05/06 18:01:43 I assume these auto margins are to center? Does fl I assume these auto margins are to center? Does flexbox's align-items or align-content also work? (If your container is already a flexbox that is) I find that more explicit. dpapad 2016/05/06 18:33:29 Removed. You are right, align-items: center, works Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > I assume these auto margins are to center? Does flexbox's align-items or align-content also work? (If your container is already a flexbox that is) I find that more explicit. Removed. You are right, align-items: center, works and settings-box already does that, so these margins are not needed anymore.
	18 }

	19 img {

	20 -webkit-margin-end: 10px;

	21 }

	22 </style>

	23 <settings-section page-title="$i18n{aboutPageTitle}"

	24 current-route="[[currentRoute]]" section="">
	tommycli 2016/05/06 18:01:43 Since Michael rewrote settings-section animations, Since Michael rewrote settings-section animations, i'm not sure what the effect of section="" is. I'm okay with it if it seems to work., this is just a warning that I'm not sure what the behavior is now. tommycli 2016/05/06 18:01:43 Additionally, I'm not sure if current-route="[[cur Additionally, I'm not sure if current-route="[[currentRoute]]" is needed anymore. Looking at the source, it kind of seems that settings-section has been made "dumb" and no longer needs the route. (And main_page_behavior handles that). Just a nit. dpapad 2016/05/06 18:33:29 Removed section="" completely. Navigation still se Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > Since Michael rewrote settings-section animations, i'm not sure what the effect of section="" is. I'm okay with it if it seems to work., this is just a warning that I'm not sure what the behavior is now. Removed section="" completely. Navigation still seems to work fine. Regarding current-route, I see that all settings-sections still use it, see https://code.google.com/p/chromium/codesearch#search/&q=settings-section%20fi.... If it is not needed we can remove it from all <settings-section> instances in a separate CL.
	25 <div class="settings-box">

	26 <img id="product-logo"

	27 srcset="chrome://theme/current-channel-logo@1x 1x,

	28 chrome://theme/current-channel-logo@2x 2x" alt="">

	29 <span>$i18n{aboutProductTitle}</span>

	30 </div>

	31 <div class="settings-box two-line">

	32 <!-- TODO(dpapad): Implement this. -->

	33 <div class="start">

	34 <div>Your device is up to date</div>
	tommycli 2016/05/06 18:01:43 If there are mocks and these are the final English If there are mocks and these are the final English strings, it would be nice if there were as few un-internationalized strings within the committed patches as possible. dpapad 2016/05/06 18:33:29 The final string is a combination of dynamic and s Show quoted text On 2016/05/06 at 18:01:43, tommycli wrote: > If there are mocks and these are the final English strings, it would be nice if there were as few un-internationalized strings within the committed patches as possible. The final string is a combination of dynamic and static data For Chrome: "Google Chrome is up to date" For Chromium: "Chromium is up to date" For Chrome OS: "Your device is up to date" I prefer to localize all the strings of this page in a separate CL. I can land that separate CL before this one or after, don't mind much. Let me know if you have a preference. tommycli 2016/05/06 18:50:20 Ah that's fine then. I'm okay with localizing in s Show quoted text On 2016/05/06 18:33:29, dpapad wrote: > On 2016/05/06 at 18:01:43, tommycli wrote: > > If there are mocks and these are the final English strings, it would be nice > if there were as few un-internationalized strings within the committed patches > as possible. > > The final string is a combination of dynamic and static data > For Chrome: "Google Chrome is up to date" > For Chromium: "Chromium is up to date" > For Chrome OS: "Your device is up to date" > > I prefer to localize all the strings of this page in a separate CL. I can land > that separate CL before this one or after, don't mind much. Let me know if you > have a preference. Ah that's fine then. I'm okay with localizing in separate CL
	35 <div class="secondary">Version Foo.bar.baz</div>

	36 </div>

	37 <paper-button class="secondary-action">Check for update</paper-button>

	38 </div>

	39 <div class="settings-box two-line">

	40 <!-- TODO(dpapad): Implement this. -->

	41 <div class="start">

	42 <div>Get help with Chrome</div>

	43 <div class="secondary">Open the Help Center</div>

	44 </div>

	45 </div>

	46 <div class="settings-box">

	47 <!-- TODO(dpapad): Implement this. -->

	48 Report an issue

	49 </div>

	50 <div class="settings-box">

	51 <!-- TODO(dpapad): Implement this. -->

	52 Other info here

	53 </div>

	54 </settings-section>

	55 </template>

	56 <script src="about_page.js"></script>

	57 </dom-module>

OLD	NEW

« no previous file with comments | « no previous file | chrome/browser/resources/settings/about_page/about_page.js » ('j') | chrome/browser/resources/settings/about_page/about_page.js » ('J')