Issue 2398383002: ChromeOS: Update Welcome screen material design OOBE.

Issue 2398383002: ChromeOS: Update Welcome screen material design OOBE. (Closed)

Created:
4 years, 2 months ago by Alexander Alekseev

Modified:
4 years, 2 months ago

Reviewers:
michaelpg

CC:
chromium-reviews, alemate+watch_chromium.org, achuith+watch_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

ChromeOS: Update Welcome screen material design OOBE. This CL updates Welcome screen to match the new mocks. BUG=604119 TEST=none CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e285449e2f13d8bd0a931a6becdee1a0f2d2cb48 Cr-Commit-Position: refs/heads/master@{#425890}

Patch Set 1 #

Patch Set 2 : Fixed Roboto-Meduim font and several font sizes. #

Total comments: 20

Patch Set 3 : Rebased. #

Patch Set 4 : Update after review. #

Patch Set 5 : Update after review. #

Created: 4 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+211 lines, -134 lines)			Patch
M	chrome/browser/resources/chromeos/login/custom_elements_oobe.html	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/custom_elements_oobe.js	View		1 chunk	+1 line, -0 lines	0 comments	Download
D	chrome/browser/resources/chromeos/login/images/welcome_illustration_1x.png	View		Binary file		0 comments	Download
A	chrome/browser/resources/chromeos/login/images/welcome_illustration_2x.png	View		Binary file		0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_buttons.html	View	1	1 chunk	+26 lines, -30 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_buttons.js	View		2 chunks	+6 lines, -19 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_dialog_host.css	View	1 2 3	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_eula.css	View	1 2 3	1 chunk	+6 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_eula.html	View	1 2 3	1 chunk	+3 lines, -1 line	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_i18n_dropdown.css	View	1 2 3	1 chunk	+11 lines, -1 line	0 comments	Download
D	chrome/browser/resources/chromeos/login/oobe_icon_button.css	View		1 chunk	+0 lines, -9 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_screen.css	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_text_button.css	View	1	2 chunks	+8 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_welcome.css	View		1 chunk	+0 lines, -39 lines	0 comments	Download
M	chrome/browser/resources/chromeos/login/oobe_welcome.html	View	1 2 3	3 chunks	+10 lines, -32 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/oobe_welcome_dialog.css	View		1 chunk	+32 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/oobe_welcome_dialog.html	View	1 2 3 4	1 chunk	+41 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/oobe_welcome_dialog.js	View		1 chunk	+28 lines, -0 lines	0 comments	Download
A	chrome/browser/resources/chromeos/login/oobe_welcome_secondary_button.css	View	1	1 chunk	+32 lines, -0 lines	0 comments	Download

Messages

Total messages: 18 (8 generated)

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

michaelpg

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/login_non_lock_shared.html File chrome/browser/resources/chromeos/login/login_non_lock_shared.html (right): https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/login_non_lock_shared.html#newcode6 chrome/browser/resources/chromeos/login/login_non_lock_shared.html:6: <!-- There is no way to include custom style, ...

4 years, 2 months ago (2016-10-09 21:02:51 UTC) #8

Alexander Alekseev

4 years, 2 months ago (2016-10-11 00:01:53 UTC) #9

michaelpg

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/oobe_buttons.html File chrome/browser/resources/chromeos/login/oobe_buttons.html (right): https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/oobe_buttons.html#newcode24 chrome/browser/resources/chromeos/login/oobe_buttons.html:24: <dom-module id="oobe-text-button"> On 2016/10/11 00:01:53, Alexander Alekseev wrote: > ...

4 years, 2 months ago (2016-10-14 16:45:21 UTC) #10

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_buttons.html (right):

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_buttons.html:24: <dom-module
id="oobe-text-button">
On 2016/10/11 00:01:53, Alexander Alekseev wrote:
> On 2016/10/09 21:02:51, michaelpg (OOO M-W) wrote:
> > why not just use <paper-button> with a class that you provide styling for? I
> > don't see this wrapper adding anything, and the cost is clear (complexity,
> extra
> > DOM nodes, copying code for focus() and onClick_())
> 
> The basic idea was to group elements by type and have only one implementation.
I
> think it is a very polymer-like way to implement UI, isn't it? ;)

Not when it comes to wrapping elements for the sake of wrapping; there's nothing
private or special about this implementation.

I'm not sure what you mean by grouping elements and having only one
implementation. oobe-text-button and oobe-welcome-secondary-button both have
implementations that amount to just wrapping the buttons (and why doesn't
oobe-welcome-secondary-button have focus() or onClick overrides?)

> 
> I could probably rename it to 'oobe-navigational-button' as it is used as
"Next"
> button only (but I am going to add some "back" instances as well).
> 
>

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome_dialog.html (right):

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome_dialog.html:19: <img
id="welcomeIllustration" src="images/welcome_illustration_2x.png">
On 2016/10/11 00:01:53, Alexander Alekseev wrote:
> On 2016/10/09 21:02:51, michaelpg (OOO M-W) wrote:
> > shouldn't this depend on screen density?
> 
> Yes, it should. But this is going to be another CL.

add a TODO

Alexander Alekseev

4 years, 2 months ago (2016-10-15 00:23:17 UTC) #11

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_buttons.html (right):

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_buttons.html:24: <dom-module
id="oobe-text-button">
On 2016/10/14 16:45:21, michaelpg (OOO M-W) wrote:
> On 2016/10/11 00:01:53, Alexander Alekseev wrote:
> > On 2016/10/09 21:02:51, michaelpg (OOO M-W) wrote:
> > > why not just use <paper-button> with a class that you provide styling for?
I
> > > don't see this wrapper adding anything, and the cost is clear (complexity,
> > extra
> > > DOM nodes, copying code for focus() and onClick_())
> > 
> > The basic idea was to group elements by type and have only one
implementation.
> I
> > think it is a very polymer-like way to implement UI, isn't it? ;)
> 
> Not when it comes to wrapping elements for the sake of wrapping; there's
nothing
> private or special about this implementation.
> 
> I'm not sure what you mean by grouping elements and having only one
> implementation. oobe-text-button and oobe-welcome-secondary-button both have
> implementations that amount to just wrapping the buttons

I believe that it is much better to make design structure clearly visible in
implementation.
If designer calls these two (actually three) buttons as "secondary buttons", we
should have them in implementation as "secondary buttons". HTML templates allow
us to have them as separate objects, which is very nice.

It also simplifies and localizes styles for this element.

> (and why doesn't oobe-welcome-secondary-button have focus() or onClick
> overrides?)

I believe this affects mostly accessibility functions and we didn't test it yet.
I think it is better to land changes in reasonably sized changes.

As soon as this is landed, we can update dependent screens and then fix issues
with different screen PPI, keyboard navigation, etc.

> > I could probably rename it to 'oobe-navigational-button' as it is used as
> "Next"
> > button only (but I am going to add some "back" instances as well).

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_welcome_dialog.html (right):

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_welcome_dialog.html:19: <img
id="welcomeIllustration" src="images/welcome_illustration_2x.png">
On 2016/10/14 16:45:21, michaelpg (OOO M-W) wrote:
> On 2016/10/11 00:01:53, Alexander Alekseev wrote:
> > On 2016/10/09 21:02:51, michaelpg (OOO M-W) wrote:
> > > shouldn't this depend on screen density?
> > 
> > Yes, it should. But this is going to be another CL.
> 
> add a TODO

Done.

michaelpg

lgtm https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/oobe_buttons.html File chrome/browser/resources/chromeos/login/oobe_buttons.html (right): https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resources/chromeos/login/oobe_buttons.html#newcode24 chrome/browser/resources/chromeos/login/oobe_buttons.html:24: <dom-module id="oobe-text-button"> On 2016/10/15 00:23:16, Alexander Alekseev wrote: ...

4 years, 2 months ago (2016-10-16 22:49:24 UTC) #12

lgtm

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/chromeos/login/oobe_buttons.html (right):

https://codereview.chromium.org/2398383002/diff/20001/chrome/browser/resource...
chrome/browser/resources/chromeos/login/oobe_buttons.html:24: <dom-module
id="oobe-text-button">
On 2016/10/15 00:23:16, Alexander Alekseev wrote:
> On 2016/10/14 16:45:21, michaelpg (OOO M-W) wrote:
> > On 2016/10/11 00:01:53, Alexander Alekseev wrote:
> > > On 2016/10/09 21:02:51, michaelpg (OOO M-W) wrote:
> > > > why not just use <paper-button> with a class that you provide styling
for?
> I
> > > > don't see this wrapper adding anything, and the cost is clear
(complexity,
> > > extra
> > > > DOM nodes, copying code for focus() and onClick_())
> > > 
> > > The basic idea was to group elements by type and have only one
> implementation.
> > I
> > > think it is a very polymer-like way to implement UI, isn't it? ;)
> > 
> > Not when it comes to wrapping elements for the sake of wrapping; there's
> nothing
> > private or special about this implementation.
> > 
> > I'm not sure what you mean by grouping elements and having only one
> > implementation. oobe-text-button and oobe-welcome-secondary-button both have
> > implementations that amount to just wrapping the buttons
> 
> I believe that it is much better to make design structure clearly visible in
> implementation.
> If designer calls these two (actually three) buttons as "secondary buttons",
we
> should have them in implementation as "secondary buttons". HTML templates
allow
> us to have them as separate objects, which is very nice.

OK. I don't agree when it comes to elements this simple -- this is what CSS
classes are designed for -- but won't press it further (and it's not a huge deal
either way).

> 
> It also simplifies and localizes styles for this element.
> 
> > (and why doesn't oobe-welcome-secondary-button have focus() or onClick
> > overrides?)
> 
> I believe this affects mostly accessibility functions and we didn't test it
yet.
> I think it is better to land changes in reasonably sized changes.
> 
> As soon as this is landed, we can update dependent screens and then fix issues
> with different screen PPI, keyboard navigation, etc.

SGTM, as long as we understand we can't ship this UI as default until it's
sufficiently accessible.

> 
> > > I could probably rename it to 'oobe-navigational-button' as it is used as
> > "Next"
> > > button only (but I am going to add some "back" instances as well).
>

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2398383002/80001

4 years, 2 months ago (2016-10-18 04:05:06 UTC) #15

commit-bot: I haz the power

4 years, 2 months ago (2016-10-18 05:53:08 UTC) #18

Message was sent while issue was closed.

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

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