Issue 1368733002: Introduce cr-settings-input and eliminate cr-input

Issue 1368733002: Introduce cr-settings-input and eliminate cr-input (Closed)

Created:
5 years, 3 months ago by stevenjb

Modified:
5 years, 3 months ago

Reviewers:
dschuyler, michaelpg

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

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

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Introduce cr-settings-input and eliminate cr-input This CL: * Eliminates cr-input * Introduces cr-settings-input for input associated with a pref * Uses cr-settings-input where appropriate and replaces other instances of cr-input with paper-input BUG=532540 Committed: https://crrev.com/ae407c9b489ad6e47ffcd8a2f570d4af1adbbc24 Cr-Commit-Position: refs/heads/master@{#350697}

Patch Set 1 #

Patch Set 2 : . #

Total comments: 16

Patch Set 3 : Elim unuseful bool support + nits #

Total comments: 10

Patch Set 4 : More feedback #

Total comments: 6

Patch Set 5 : Propagate readonly + feedback #

Patch Set 6 : Rebase #

Created: 5 years, 3 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+224 lines, -239 lines)			Patch
M	chrome/browser/resources/settings/appearance_page/appearance_page.html	View	1 2 3 4	2 chunks	+4 lines, -3 lines	0 comments	Download
M	chrome/browser/resources/settings/downloads_page/downloads_page.css	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/settings/downloads_page/downloads_page.html	View	1 2 3 4	2 chunks	+5 lines, -6 lines	0 comments	Download
A +	chrome/browser/resources/settings/input/input.css	View		0 chunks	+-1 lines, --1 lines	0 comments	Download
A +	chrome/browser/resources/settings/input/input.html	View	1 2 3 4	1 chunk	+10 lines, -10 lines	0 comments	Download
A	chrome/browser/resources/settings/input/input.js	View	1 2 3 4	1 chunk	+138 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_proxy.css	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_proxy.html	View	1 2 3	3 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_proxy_input.css	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_proxy_input.html	View		2 chunks	+5 lines, -5 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_siminfo.css	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/settings/internet_page/network_siminfo.html	View		5 chunks	+17 lines, -17 lines	0 comments	Download
M	chrome/browser/resources/settings/search_engines_page/search_engine_adder.html	View		2 chunks	+7 lines, -7 lines	0 comments	Download
M	chrome/browser/resources/settings/search_engines_page/search_engine_entry.css	View		1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/settings/search_engines_page/search_engine_entry.html	View		1 chunk	+13 lines, -11 lines	0 comments	Download
M	chrome/browser/resources/settings/search_engines_page/search_engine_entry.js	View		2 chunks	+1 line, -2 lines	0 comments	Download
M	chrome/browser/resources/settings/search_engines_page/search_engines_list.html	View		1 chunk	+0 lines, -1 line	0 comments	Download
M	chrome/browser/resources/settings/settings_resources.grd	View		1 chunk	+9 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/settings/users_page/users_page.css	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/settings/users_page/users_page.html	View		2 chunks	+3 lines, -3 lines	0 comments	Download
D	ui/webui/resources/cr_elements/v1_0/cr_input/cr_input.css	View		1 chunk	+0 lines, -7 lines	0 comments	Download
D	ui/webui/resources/cr_elements/v1_0/cr_input/cr_input.html	View		1 chunk	+0 lines, -17 lines	0 comments	Download
D	ui/webui/resources/cr_elements/v1_0/cr_input/cr_input.js	View		1 chunk	+0 lines, -96 lines	0 comments	Download
D	ui/webui/resources/cr_elements/v1_0/cr_input/demo.html	View		1 chunk	+0 lines, -15 lines	0 comments	Download
M	ui/webui/resources/cr_elements/v1_0/demo_element.html	View		1 chunk	+0 lines, -18 lines	0 comments	Download
M	ui/webui/resources/cr_elements_resources.grdp	View		1 chunk	+0 lines, -9 lines	0 comments	Download

Messages

Total messages: 19 (6 generated)

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

michaelpg

https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js File chrome/browser/resources/settings/input/input.js (right): https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js#newcode12 chrome/browser/resources/settings/input/input.js:12: * <cr-settings-input></cr-settings-input> exclude useless example IMO https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js#newcode72 chrome/browser/resources/settings/input/input.js:72: focus: ...

5 years, 3 months ago (2015-09-24 18:59:31 UTC) #3

stevenjb

https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js File chrome/browser/resources/settings/input/input.js (right): https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js#newcode12 chrome/browser/resources/settings/input/input.js:12: * <cr-settings-input></cr-settings-input> On 2015/09/24 18:59:31, michaelpg wrote: > exclude ...

5 years, 3 months ago (2015-09-24 19:28:53 UTC) #4

michaelpg

https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js File chrome/browser/resources/settings/input/input.js (right): https://codereview.chromium.org/1368733002/diff/20001/chrome/browser/resources/settings/input/input.js#newcode119 chrome/browser/resources/settings/input/input.js:119: this.set('pref.value', this.value == 'true'); On 2015/09/24 19:28:53, stevenjb wrote: ...

5 years, 3 months ago (2015-09-24 19:42:15 UTC) #5

stevenjb

dschuyler@ - I think this CL is worth having two sets fo eyes on, so ...

5 years, 3 months ago (2015-09-24 19:59:59 UTC) #6

michaelpg

lgtm, but i agree wait for dschuyler@ https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html#newcode15 chrome/browser/resources/settings/downloads_page/downloads_page.html:15: <cr-settings-input id="downloadsPath" ...

5 years, 3 months ago (2015-09-24 20:03:54 UTC) #7

stevenjb

https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html File chrome/browser/resources/settings/downloads_page/downloads_page.html (right): https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html#newcode15 chrome/browser/resources/settings/downloads_page/downloads_page.html:15: <cr-settings-input id="downloadsPath" readonly On 2015/09/24 20:03:54, michaelpg wrote: > ...

5 years, 3 months ago (2015-09-24 20:06:47 UTC) #8

dschuyler

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resources/settings/appearance_page/appearance_page.html File chrome/browser/resources/settings/appearance_page/appearance_page.html (right): https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resources/settings/appearance_page/appearance_page.html#newcode39 chrome/browser/resources/settings/appearance_page/appearance_page.html:39: </cr-settings> Should this be </cr-settings-input> ? https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resources/settings/input/input.html File chrome/browser/resources/settings/input/input.html ...

5 years, 3 months ago (2015-09-24 20:39:06 UTC) #9

stevenjb

5 years, 3 months ago (2015-09-24 21:22:15 UTC) #10

https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/downloads_page/downloads_page.html
(right):

https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:15:
<cr-settings-input id="downloadsPath" readonly
On 2015/09/24 20:06:47, stevenjb wrote:
> On 2015/09/24 20:03:54, michaelpg wrote:
> > On 2015/09/24 19:59:59, stevenjb wrote:
> > > On 2015/09/24 19:42:14, michaelpg wrote:
> > > > readonly -> disabled?
> > > 
> > > I just moved this; I don't want to make any changes.
> > 
> > meh, a bug's a bug
> 
> Not really a bug though, it's a UX thing, right? I'm not familiar enough with
> the design to make the call :P

So, readonly is valid, but we need to propagate it, and that is a bug, so I
fixed that.

I'll let whomever owns the page work with UX as to whether read-only or disabled
is more correct.

https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
floating-label="false"
On 2015/09/24 19:59:59, stevenjb wrote:
> On 2015/09/24 19:42:14, michaelpg wrote:
> > is floatingLabel a thing? (and if so shouldn't we remove the attribute
> entirely
> > to set it to "false", otherwise it's set to true by virtue of having a
value)
> 
> floating-label does not appear to be a thing, I'll go ahead and remove that.

This is probably supposed to be no-label-float, so changed to that.

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/appearance_page/appearance_page.html
(right):

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/appearance_page/appearance_page.html:39:
</cr-settings>
On 2015/09/24 20:39:05, dschuyler wrote:
> Should this be </cr-settings-input> ?

Oops, yes, good catch. Done.

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/input/input.html (right):

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/input/input.html:2: <link rel="import"
href="chrome://resources/polymer/v1_0/paper-input/paper-input.html">
On 2015/09/24 20:39:05, dschuyler wrote:
> Maybe consider using a higher 'similarity' to avoid connecting new files to
> unrelated files.  e.g. git cl upload --similarity=70
> (the default is 50%, and these two are likely over 50% similar).

*shrug* They are in fact similar enough that I find the diff if anything to be
helpful.

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/input/input.js (right):

https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/input/input.js:109: return;  // Ignore empty
input field
On 2015/09/24 20:39:05, dschuyler wrote:
> Maybe set this.value to pref.value if value is empty?
> That's kind of what's happening behind the scenes,
> I think I'm just suggesting that relay the state
> to the user.

Eventually we should do proper validation, but yeah, that's probably better for
now.
Done.

dschuyler

On 2015/09/24 21:22:15, stevenjb wrote: > https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html > File chrome/browser/resources/settings/downloads_page/downloads_page.html > (right): > > https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resources/settings/downloads_page/downloads_page.html#newcode15 ...

5 years, 3 months ago (2015-09-24 21:42:21 UTC) #11

On 2015/09/24 21:22:15, stevenjb wrote:
>
https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
> File chrome/browser/resources/settings/downloads_page/downloads_page.html
> (right):
> 
>
https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/downloads_page/downloads_page.html:15:
> <cr-settings-input id="downloadsPath" readonly
> On 2015/09/24 20:06:47, stevenjb wrote:
> > On 2015/09/24 20:03:54, michaelpg wrote:
> > > On 2015/09/24 19:59:59, stevenjb wrote:
> > > > On 2015/09/24 19:42:14, michaelpg wrote:
> > > > > readonly -> disabled?
> > > > 
> > > > I just moved this; I don't want to make any changes.
> > > 
> > > meh, a bug's a bug
> > 
> > Not really a bug though, it's a UX thing, right? I'm not familiar enough
with
> > the design to make the call :P
> 
> So, readonly is valid, but we need to propagate it, and that is a bug, so I
> fixed that.
> 
> I'll let whomever owns the page work with UX as to whether read-only or
disabled
> is more correct.
> 
>
https://codereview.chromium.org/1368733002/diff/40001/chrome/browser/resource...
> chrome/browser/resources/settings/downloads_page/downloads_page.html:16:
> floating-label="false"
> On 2015/09/24 19:59:59, stevenjb wrote:
> > On 2015/09/24 19:42:14, michaelpg wrote:
> > > is floatingLabel a thing? (and if so shouldn't we remove the attribute
> > entirely
> > > to set it to "false", otherwise it's set to true by virtue of having a
> value)
> > 
> > floating-label does not appear to be a thing, I'll go ahead and remove that.
> 
> This is probably supposed to be no-label-float, so changed to that.
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/appearance_page/appearance_page.html
> (right):
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/appearance_page/appearance_page.html:39:
> </cr-settings>
> On 2015/09/24 20:39:05, dschuyler wrote:
> > Should this be </cr-settings-input> ?
> 
> Oops, yes, good catch. Done.
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/input/input.html (right):
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/input/input.html:2: <link rel="import"
> href="chrome://resources/polymer/v1_0/paper-input/paper-input.html">
> On 2015/09/24 20:39:05, dschuyler wrote:
> > Maybe consider using a higher 'similarity' to avoid connecting new files to
> > unrelated files.  e.g. git cl upload --similarity=70
> > (the default is 50%, and these two are likely over 50% similar).
> 
> *shrug* They are in fact similar enough that I find the diff if anything to be
> helpful.
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> File chrome/browser/resources/settings/input/input.js (right):
> 
>
https://codereview.chromium.org/1368733002/diff/60001/chrome/browser/resource...
> chrome/browser/resources/settings/input/input.js:109: return;  // Ignore empty
> input field
> On 2015/09/24 20:39:05, dschuyler wrote:
> > Maybe set this.value to pref.value if value is empty?
> > That's kind of what's happening behind the scenes,
> > I think I'm just suggesting that relay the state
> > to the user.
> 
> Eventually we should do proper validation, but yeah, that's probably better
for
> now.
> Done.

LGTM

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1368733002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1368733002/100001

5 years, 3 months ago (2015-09-24 21:57:35 UTC) #17

commit-bot: I haz the power

5 years, 3 months ago (2015-09-24 23:27:00 UTC) #19

Message was sent while issue was closed.

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

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