chrome/browser/resources/settings/multidevice_page/multidevice_page.html - Issue 2964293002: CrOS Settings: Add skeleton page for multidevice section.

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

Issue 2964293002: CrOS Settings: Add skeleton page for multidevice section. (Closed)

Patch Set: Minor cleanups Created 3 years, 5 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:

« chrome/browser/resources/settings/basic_page/basic_page.js ('K') | « chrome/browser/resources/settings/multidevice_page/compiled_resources2.gyp ('k') | chrome/browser/resources/settings/multidevice_page/multidevice_page.js » ('j') | chrome/test/data/webui/settings/cr_settings_browsertest.js » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

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

	2

	3 <link rel="import" href="chrome://resources/cr_elements/policy/cr_policy_pref_in dicator.html">
	dpapad 2017/07/06 01:26:50 Probably copy pasted from elsewhere? Let's remove Probably copy pasted from elsewhere? Let's remove dependencies that are not currently being used in this file. Oren Blasberg 2017/07/06 18:17:59 Done. Show quoted text On 2017/07/06 01:26:50, dpapad wrote: > Probably copy pasted from elsewhere? Let's remove dependencies that are not > currently being used in this file. Done.
	4 <link rel="import" href="chrome://resources/polymer/v1_0/neon-animation/neon-ani matable.html">

	5 <link rel="import" href="chrome://resources/polymer/v1_0/paper-icon-button/paper -icon-button-light.html">

	6 <link rel="import" href="chrome://resources/polymer/v1_0/paper-toggle-button/pap er-toggle-button.html">

	7 <link rel="import" href="../i18n_setup.html">

	8 <link rel="import" href="../icons.html">

	9 <link rel="import" href="../prefs/prefs.html">

	10 <link rel="import" href="../settings_page/settings_animated_pages.html">

	11 <link rel="import" href="../settings_page/settings_subpage.html">

	12 <link rel="import" href="../settings_shared_css.html">

	13

	14 <dom-module id="settings-multidevice-page">

	15 <template>

	16 <style include="settings-shared">

	17 </style>
	dpapad 2017/07/06 01:26:50 Nit: Can fit in previous line. Nit: Can fit in previous line. Oren Blasberg 2017/07/06 18:17:58 Done. Show quoted text On 2017/07/06 01:26:50, dpapad wrote: > Nit: Can fit in previous line. Done.
	18 <settings-animated-pages id="pages" section="multidevice">

	19 <neon-animatable route-path="default">
	dpapad 2017/07/06 01:26:50 I am a bit torn on how to review this file. A lot I am a bit torn on how to review this file. A lot of the HTML markup is currently unused. Can we make this as minimal as possible to serve its current purpose (being a dummy placeholder)? Specifically, how about removing all IDs and CSS classes that are not used. They can be added when this UI component gets more fleshed out. Oren Blasberg 2017/07/06 18:17:59 Sure. Can you point out which IDs and classes are Show quoted text On 2017/07/06 01:26:50, dpapad wrote: > I am a bit torn on how to review this file. A lot of the HTML markup is > currently unused. Can we make this as minimal as possible to serve its current > purpose (being a dummy placeholder)? > > Specifically, how about removing all IDs and CSS classes that are not used. They > can be added when this UI component gets more fleshed out. Sure. Can you point out which IDs and classes are ok to keep and which to remove? I've removed id="enableSmsConnect" on the paper-toggle-button since no one is referencing that. The rest of the css classes seem to be just styling it so the setting looks visually consistent with Material. Is it OK to leave those?
	20 <div id="multidevice" class="settings-box two-line">

	21 <iron-icon icon="settings:sync"></iron-icon>

	22 <div id="smsConnectToggleLabel" class="middle">

	23 $i18n{smsConnect}

	24 </div>

	25 <div class="separator"></div>

	26 <paper-toggle-button id="enableSmsConnect"
	dpapad 2017/07/06 01:26:50 Related to the comment above, this is unlikely to Related to the comment above, this is unlikely to stay as is, as this file gets more concrete. For example, if you want a toggle button that automatically flips a pref, there is already an element for that, called settings-toggle-button, see examples at [1] below. Using that element instead, eliminates the need for the TODO you are adding in multidevice_page.js, or to be more precise, the code that propagates the pref change to the appropriate app would probably reside in C++ instead. Not sure if that element throws errors if hooked up to a non-existing (for now) pref though. If you want to keep this code as is, then I suggest to at least update the TODO in the JS file to mention that a settings-toggle-button should be used eventually. [1] https://cs.chromium.org/search/?q=settings-toggle-button+file:%5Esrc/chrome/b... Oren Blasberg 2017/07/06 18:17:59 Yes exactly, I can't use settings-toggle-button un Show quoted text On 2017/07/06 01:26:50, dpapad wrote: > Related to the comment above, this is unlikely to stay as is, as this file gets > more concrete. > > For example, if you want a toggle button that automatically flips a pref, there > is already an element for that, called settings-toggle-button, see examples at > [1] below. Using that element instead, eliminates the need for the TODO you are > adding in multidevice_page.js, or to be more precise, the code that propagates > the pref change to the appropriate app would probably reside in C++ instead. > > Not sure if that element throws errors if hooked up to a non-existing (for now) > pref though. If you want to keep this code as is, then I suggest to at least > update the TODO in the JS file to mention that a settings-toggle-button should > be used eventually. > > [1] > https://cs.chromium.org/search/?q=settings-toggle-button+file:%5Esrc/chrome/b... Yes exactly, I can't use settings-toggle-button until there is an actual pref there. It throws errors without a pref= attribute specified. Updated the TODO in the js accordingly. Thanks :)
	27 checked="{{smsConnectToggleState_}}"

	28 aria-labelledby="smsConnectToggleLabel">

	29 </paper-toggle-button>

	30 </div>

	31 </neon-animatable>

	32 </settings-animated-pages>

	33 </template>

	34 <script src="multidevice_page.js"></script>

	35 </dom-module>

OLD	NEW