Issue 990473002: Add initial implementation of date-time-settings MD settings page.

Issue 990473002: Add initial implementation of date-time-settings MD settings page. (Closed)

Created:
5 years, 9 months ago by Oren Blasberg

Modified:
5 years, 8 months ago

Reviewers:
stevenjb, Jeremy Klein, James Hawkins, michaelpg

CC:
chromium-reviews, khorimoto+watch-md-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, jhawkins+watch-md-settings_chromium.org, orenb+watch-md-settings_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dcheng, stevenjb+watch-md-settings_chromium.org, jlklein+watch-md-settings_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add initial implementation of date-time-settings MD settings page. Also, I fixed two issues in a11y page: 1. settings. -> settings.settings. (b/c the names were already prefixed with "settings.") 2. Correctly concat the list of prefs to request via chrome.send() (this will go away later anyway when we switch to settingsPrivate API.) BUG=464898 Committed: https://crrev.com/c3a23f04cab52b8e5ba02a724237fcb65bb0e354 Cr-Commit-Position: refs/heads/master@{#324163}

Patch Set 1 #

Patch Set 2 : Fix a few bugs. #

Patch Set 3 : Clean up a couple things #

Total comments: 19

Patch Set 4 : Respond to comments #

Patch Set 5 : Update layout to reflect new changes. #

Total comments: 17

Patch Set 6 : Address michael's cmts #

Patch Set 7 : Respond to jeremys cmts #

Total comments: 1

Patch Set 8 : #

Total comments: 5

Patch Set 9 : Fixing issues #

Total comments: 11

Patch Set 10 : Address nits #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+92 lines, -31 lines)			Patch
M	chrome/browser/resources/settings/a11y_page/a11y_page.html	View	1 2 3 4 5 6 7 8	2 chunks	+17 lines, -11 lines	0 comments	Download
A +	chrome/browser/resources/settings/date_time_page/date_time_page.css	View	1 2 3 4 5	1 chunk	+8 lines, -4 lines	0 comments	Download
A	chrome/browser/resources/settings/date_time_page/date_time_page.html	View	1 2 3 4 5 6 7 8	1 chunk	+27 lines, -0 lines	0 comments	Download
A +	chrome/browser/resources/settings/date_time_page/date_time_page.js	View	1 2 3 4 5 6 7 8	2 chunks	+11 lines, -9 lines	0 comments	Download
A +	chrome/browser/resources/settings/date_time_page/demo.html	View	5 6 7 8	1 chunk	+2 lines, -2 lines	0 comments	Download
A +	chrome/browser/resources/settings/date_time_page/demo.js	View	1 2 3	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/settings/pref_tracker/pref_tracker.js	View	1 2 3 4 5 6 7 8	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/settings/prefs/prefs.js	View	1 2 3 4 5 6 7 8	3 chunks	+3 lines, -1 line	0 comments	Download
M	chrome/browser/resources/settings/settings_main/settings_main.html	View	1 2 3 4 5 6 7 8 9	2 chunks	+5 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/settings_page/settings_page_header.html	View	1 2 3 4 5 6 7 8 9	1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/settings_resources.grd	View	1 2 3 4 5 6 7 8	2 chunks	+12 lines, -1 line	0 comments	Download
M	ui/webui/resources/polymer_resources.grdp	View	1 2 3 4 5 6 7 8	1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 31 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Jeremy Klein

https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode30 chrome/browser/resources/settings/date_time_page/date_time_page.html:30: <cr-checkbox checked="{{prefs.settings.dateTime.use24HourClock}}" for> It seems like these don't actually ...

5 years, 9 months ago (2015-03-06 23:26:45 UTC) #3

michaelpg

https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.css File chrome/browser/resources/settings/date_time_page/date_time_page.css (right): https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.css#newcode17 chrome/browser/resources/settings/date_time_page/date_time_page.css:17: cr-input { some of this looks copy-pasted from downloads_page.css ...

5 years, 9 months ago (2015-03-08 05:57:08 UTC) #4

Oren Blasberg

https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.css File chrome/browser/resources/settings/date_time_page/date_time_page.css (right): https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.css#newcode17 chrome/browser/resources/settings/date_time_page/date_time_page.css:17: cr-input { On 2015/03/08 05:57:08, michaelpg wrote: > some ...

5 years, 9 months ago (2015-03-10 01:25:01 UTC) #5

michaelpg

lgtm https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode20 chrome/browser/resources/settings/date_time_page/date_time_page.html:20: Pacific Standard Time On 2015/03/10 01:25:01, Oren Blasberg ...

5 years, 9 months ago (2015-03-10 03:22:29 UTC) #6

Oren Blasberg

I updated things so the layout fits into the scaffold ui now PTAL https://codereview.chromium.org/990473002/diff/40001/chrome/browser/resources/settings/date_time_page/date_time_page.html File ...

5 years, 9 months ago (2015-03-24 00:37:48 UTC) #7

michaelpg

Looking good, will make another pass tonight. https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.css File chrome/browser/resources/settings/date_time_page/date_time_page.css (right): https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.css#newcode13 chrome/browser/resources/settings/date_time_page/date_time_page.css:13: cr-checkbox { ...

5 years, 9 months ago (2015-03-24 00:43:38 UTC) #8

Jeremy Klein

https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode35 chrome/browser/resources/settings/date_time_page/date_time_page.html:35: pref="{{prefs.settings.clock.use24HourClock}}" camelCase -> under-scores https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode35 chrome/browser/resources/settings/date_time_page/date_time_page.html:35: pref="{{prefs.settings.clock.use24HourClock}}" Ugh I ...

5 years, 9 months ago (2015-03-24 00:49:06 UTC) #9

Oren Blasberg

https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.css File chrome/browser/resources/settings/date_time_page/date_time_page.css (right): https://codereview.chromium.org/990473002/diff/80001/chrome/browser/resources/settings/date_time_page/date_time_page.css#newcode13 chrome/browser/resources/settings/date_time_page/date_time_page.css:13: cr-checkbox { On 2015/03/24 00:43:37, michaelpg wrote: > remove ...

5 years, 9 months ago (2015-03-24 00:54:58 UTC) #10

Jeremy Klein

lgtm with a nit. https://codereview.chromium.org/990473002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.html File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/990473002/diff/120001/chrome/browser/resources/settings/settings_main/settings_main.html#newcode18 chrome/browser/resources/settings/settings_main/settings_main.html:18: <cr-settings-date-time-page prefs="{{prefs}}"> Can you wrap ...

5 years, 9 months ago (2015-03-24 23:12:00 UTC) #11

stevenjb

https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode28 chrome/browser/resources/settings/date_time_page/date_time_page.html:28: </core-menu> The above seems pretty heavyweight for a dropdown ...

5 years, 9 months ago (2015-03-25 21:15:30 UTC) #13

Jeremy Klein

https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode28 chrome/browser/resources/settings/date_time_page/date_time_page.html:28: </core-menu> On 2015/03/25 21:15:30, stevenjb wrote: > The above ...

5 years, 9 months ago (2015-03-25 21:24:26 UTC) #14

stevenjb

lgtm w/ comment https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html File chrome/browser/resources/settings/date_time_page/date_time_page.html (right): https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html#newcode28 chrome/browser/resources/settings/date_time_page/date_time_page.html:28: </core-menu> On 2015/03/25 21:24:26, Jeremy Klein ...

5 years, 9 months ago (2015-03-25 21:55:36 UTC) #15

lgtm w/ comment

https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/settings/date_time_page/date_time_page.html
(right):

https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resource...
chrome/browser/resources/settings/date_time_page/date_time_page.html:28:
</core-menu>
On 2015/03/25 21:24:26, Jeremy Klein wrote:
> On 2015/03/25 21:15:30, stevenjb wrote:
> > The above seems pretty heavyweight for a dropdown and a label. Presumably in
> > this case we will eventually want the contents (i.e. list of time zones) to
be
> > data driven. I expect that will be generally true for other dropdowns.
> > 
> > I'd kind of prefer we implement this without the core-menu in the HTML and
add
> > the test entries in the JS, even in this initial implementation.
> 
> This will eventually be done in a cr-settings-select or something, which will
> have some attribute which is a list of options (or map from text to value).
> You're right that this will be data-driven via a <template repeat="blah">, but
> I'm not sure I understand your request to implement this without a core-menu.
> I'm pretty sure we'll need a core-menu regardless of whether it's here or
inside
> the cr-settings-select. I also think it's fine to land this as-is until we
have
> the cr-settings-select and some idea of the structure of the timezone objects
> and the api we get those from. It would be fine to do that in a followup to
the
> basic UI IMO.

I was suggesting that it should be inside cr-settings-select. At the page level
a dropdown should (ideally, imho) appear as a single element with just the id,
pref, and label attributes.

I don't have a strong objection to landing this as-is, but personally I would
prefer we omit this for now so that somebody doesn't unknowingly copy this
pattern and use it elsewhere. Unlikely, granted, but requiring a dozen or so
lines of HTML for what should be a single element is just the sort of pattern I
am hoping we can avoid.

Jeremy Klein

On 2015/03/25 21:55:36, stevenjb wrote: > lgtm w/ comment > > https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/date_time_page/date_time_page.html > File chrome/browser/resources/settings/date_time_page/date_time_page.html ...

5 years, 9 months ago (2015-03-25 21:57:28 UTC) #16

On 2015/03/25 21:55:36, stevenjb wrote:
> lgtm w/ comment
> 
>
https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resource...
> File chrome/browser/resources/settings/date_time_page/date_time_page.html
> (right):
> 
>
https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resource...
> chrome/browser/resources/settings/date_time_page/date_time_page.html:28:
> </core-menu>
> On 2015/03/25 21:24:26, Jeremy Klein wrote:
> > On 2015/03/25 21:15:30, stevenjb wrote:
> > > The above seems pretty heavyweight for a dropdown and a label. Presumably
in
> > > this case we will eventually want the contents (i.e. list of time zones)
to
> be
> > > data driven. I expect that will be generally true for other dropdowns.
> > > 
> > > I'd kind of prefer we implement this without the core-menu in the HTML and
> add
> > > the test entries in the JS, even in this initial implementation.
> > 
> > This will eventually be done in a cr-settings-select or something, which
will
> > have some attribute which is a list of options (or map from text to value).
> > You're right that this will be data-driven via a <template repeat="blah">,
but
> > I'm not sure I understand your request to implement this without a
core-menu.
> > I'm pretty sure we'll need a core-menu regardless of whether it's here or
> inside
> > the cr-settings-select. I also think it's fine to land this as-is until we
> have
> > the cr-settings-select and some idea of the structure of the timezone
objects
> > and the api we get those from. It would be fine to do that in a followup to
> the
> > basic UI IMO.
> 
> I was suggesting that it should be inside cr-settings-select. At the page
level
> a dropdown should (ideally, imho) appear as a single element with just the id,
> pref, and label attributes.
> 
> I don't have a strong objection to landing this as-is, but personally I would
> prefer we omit this for now so that somebody doesn't unknowingly copy this
> pattern and use it elsewhere. Unlikely, granted, but requiring a dozen or so
> lines of HTML for what should be a single element is just the sort of pattern
I
> am hoping we can avoid.

Oh, ok. Sounds like we're on the same page then. Removing the dropdown for now
with a TODO or something is fine with me, but I'm fine landing this and
iterating as well.

Oren Blasberg

OK, fixed all the issues. Also, I fixed two issues in a11y page: 1. settings. ...

5 years, 8 months ago (2015-04-07 01:22:34 UTC) #17

Oren Blasberg

https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/prefs/prefs_types.js File chrome/browser/resources/settings/prefs/prefs_types.js (right): https://codereview.chromium.org/990473002/diff/140001/chrome/browser/resources/settings/prefs/prefs_types.js#newcode1 chrome/browser/resources/settings/prefs/prefs_types.js:1: // Copyright 2015 The Chromium Authors. All rights reserved. ...

5 years, 8 months ago (2015-04-07 01:22:40 UTC) #18

Oren Blasberg

I also verified that the menu item for Date & Time is there on Chrome ...

5 years, 8 months ago (2015-04-07 02:11:56 UTC) #19

michaelpg

SLGTM pending <if> indentation, with optional suggestions. https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode23 chrome/browser/resources/settings/a11y_page/a11y_page.html:23: pref="{{prefs.settings.settings.a11y.enable_menu}}" I ...

5 years, 8 months ago (2015-04-07 06:00:30 UTC) #20

Jeremy Klein

LGTM with Michael's comments. https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode23 chrome/browser/resources/settings/a11y_page/a11y_page.html:23: pref="{{prefs.settings.settings.a11y.enable_menu}}" On 2015/04/07 06:00:29, michaelpg ...

5 years, 8 months ago (2015-04-07 17:28:03 UTC) #21

Oren Blasberg

https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html File chrome/browser/resources/settings/a11y_page/a11y_page.html (right): https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/a11y_page/a11y_page.html#newcode23 chrome/browser/resources/settings/a11y_page/a11y_page.html:23: pref="{{prefs.settings.settings.a11y.enable_menu}}" On 2015/04/07 17:28:03, Jeremy Klein wrote: > On ...

5 years, 8 months ago (2015-04-07 23:25:59 UTC) #22

commit-bot: I haz the power

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

5 years, 8 months ago (2015-04-07 23:26:56 UTC) #25

commit-bot: I haz the power

Patchset 10 (id:??) landed as https://crrev.com/c3a23f04cab52b8e5ba02a724237fcb65bb0e354 Cr-Commit-Position: refs/heads/master@{#324163}

5 years, 8 months ago (2015-04-08 00:44:00 UTC) #27

michaelpg

https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/settings_main/settings_main.html File chrome/browser/resources/settings/settings_main/settings_main.html (right): https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resources/settings/settings_main/settings_main.html#newcode17 chrome/browser/resources/settings/settings_main/settings_main.html:17: <if expr="chromeos"> On 2015/04/07 23:25:59, Oren Blasberg wrote: > ...

5 years, 8 months ago (2015-04-10 08:01:04 UTC) #28

James Hawkins

5 years, 8 months ago (2015-04-10 15:03:55 UTC) #30

chromium-reviews

5 years, 8 months ago (2015-04-10 19:30:55 UTC) #31

Message was sent while issue was closed.

On Apr 10, 2015 8:04 AM, <jhawkins@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_main/settings_main.html
> (right):
>
>
https://codereview.chromium.org/990473002/diff/160001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_main/settings_main.html:17:
> <if expr="chromeos">
> On 2015/04/10 08:01:04, michaelpg wrote:
>>
>> On 2015/04/07 23:25:59, Oren Blasberg wrote:
>> > On 2015/04/07 06:00:29, michaelpg wrote:
>> > > <if> tags should be 0-aligned, i.e. not indented at all.
>> >
>> > Done. BTW, why is that?
>
>
>> It is just The Way Of Things.
>
>
>> Interestingly, the style guide doesn't say whether or not <if> should
>
> be
>>
>> indented:
>> https://www.chromium.org/developers/web-development-style-guide
>
>
>> And the example used *does* indent the inner HTML of the <if> tag.
>
>
>> But most of chromium's HTML I've seen 0-aligns <if> tags (just like
>
> C++ #ifdef
>>
>> tags), and generally doesn't indent the inner HTML (again like #ifdef
>
> tags).
>
> That portion of the style guide is ambiguous because there is no <body>
> with which to reference the indentation of the rest of the example HTML.
> The <if> should behave like #if and similar macros in C++, i.e., zero
> indentation. The content inside the <if> should be aligned with respect
> to the surrounding HTML as though the <if> did not exist, e.g.,
>
>   <div>
> <if>
>     <span>My Content</span>
> </if>
>   </div>
>
> https://codereview.chromium.org/990473002/

It's not really ambiguous, because of the sibling h3 and div tags, right?
Considering what you described seems to be the convention in Chromium, I'd
say this is a bug in the style guide.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages