Yan Or Assignee
	Description • 10 years ago

      No description provided.

	Yan Or Assignee
	Comment 1 • 10 years ago

Kevin,

Please redirect r? as needed.  Thanks.

	Kevin Grandon :kgrandon
	Comment 2 • 10 years ago

Comment on attachment 8429820 [details]
Pull request

Hey, you should probably use the modules page (https://wiki.mozilla.org/Modules/FirefoxOS) to find the right reviewer, or use the history in github of the repo. In this case, I think it would make sense for Rudy to review this perhaps.

Rudy - could you please take a look at this? If you don't have time, feel free to delegate to me. Thanks!

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 3 • 10 years ago

Comment on attachment 8429820 [details]
Pull request

I tried to find another app that is utilizing the web component for header but found nothing, so please help me to review this with the following questions,

1. Is the whole web component thing production level?

2. Will this work when running Gaia with Firefox browser Nightly?
   From my tests, it seems not working.

	Kevin Grandon :kgrandon
	Comment 4 • 10 years ago

Comment on attachment 8429820 [details]
Pull request

(In reply to Rudy Lu [:rudyl] from comment #3)
> Comment on attachment 8429820 [details]
> Pull request
> 
> I tried to find another app that is utilizing the web component for header
> but found nothing, so please help me to review this with the following
> questions,

There are a bunch of open pull requests, and they are currently trying to land code. Whether or not this lands in 2.0 is yet to be seen.

> 1. Is the whole web component thing production level?

Basic usage is. There are a few platform bugs still, but we've found workarounds for most of them.

> 2. Will this work when running Gaia with Firefox browser Nightly?
>    From my tests, it seems not working.

It should work! You will need to set the 'dom.webcomponents.enabled' flag to true.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 5 • 10 years ago

Ok, thanks for the info.

Landed to Gaia master to see how it goes,
https://github.com/mozilla-b2g/gaia/commit/0e5d49bcc6e6501b82819dbef82a783f79a68da6

	Yan Or Assignee
	Comment 6 • 10 years ago

Wasn't expecting this to land.  Need to change the attribute names per:

https://bugzilla.mozilla.org/show_bug.cgi?id=1011807#c14

Should I reopen this ticket and update the patch or create a new patch?

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 7 • 10 years ago

(In reply to Yan Or from comment #6)
> Wasn't expecting this to land.  Need to change the attribute names per:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1011807#c14
> 
> Should I reopen this ticket and update the patch or create a new patch?

Oops, it's my fault to land this without asking for your opinion again.
With bug 1020908, do you think we should back this out first and have a new/correct patch?

	Yan Or Assignee
	Comment 8 • 10 years ago

Rudy, please back this out for now.  We most likely won't land gaia-header in 2.0.

Sorry about the confusion.

	Kevin Grandon :kgrandon
	Comment 9 • 10 years ago

Reverted: https://github.com/mozilla-b2g/gaia/commit/073f92c8c204c333170cc81dd05a20c9cdd6b57e

We should probably stop asking for reviews if we're not ready to land. Instead you can ask for feedback?

	Wilson Page [:wilsonpage]
	Comment 11 • 10 years ago

yor: I'm not sure how to test this. When/how does the keyboard have a header?

	Wilson Page [:wilsonpage]
	Comment 12 • 10 years ago

Ah found it :)

'Settings' > 'Keyboard' > 'Built-In Keyboard'

	Wilson Page [:wilsonpage]
	Comment 13 • 10 years ago

Comment on attachment 8443246 [details]
Rebased pull request

Looks fine to me. Don't want to land until we have decided where gaia-header is going to live (gaia or gaia-components). That may require some tweaks to this patch.

This patch will need some tweaks when new gaia-header lands (with centered logic). Therefore converting to f+.

	Yan Or Assignee
	Comment 14 • 10 years ago

Comment on attachment 8443246 [details]
Rebased pull request

Rebased off latest master.

Wilson, please take a quick look at this and land it if ok.

	Wilson Page [:wilsonpage]
	Comment 15 • 10 years ago

Comment on attachment 8443246 [details]
Rebased pull request

Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/a7f25f36897b98dbb11c37f55cc1bbe57fcd326d

	Kevin Grandon :kgrandon
	Comment 16 • 10 years ago

Reverted for being highly likely of causing a Gij perma-fail:

1) manipulate keyboard settings Launch built-in keyboard's settings page:
00:28:39     INFO -    NoSuchElement: (7) Unable to locate element: #back
00:28:39     INFO -    Remote Stack:
00:28:39     INFO -    <none>
00:28:39     INFO -        at Error.MarionetteError (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/error.js:67:13)
00:28:39     INFO -        at Object.Client._handleCallback (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:476:19)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:510:21
00:28:39     INFO -        at TcpSync.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/drivers/tcp-sync.js:153:10)
00:28:39     INFO -        at Object.send (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:457:36)
00:28:39     INFO -        at Object.Client._sendCommand (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:503:19)
00:28:39     INFO -        at Object._findElement (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1303:19)
00:28:39     INFO -        at Object.findElement (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:1352:32)
00:28:39     INFO -        at Object.MarionetteHelper.waitForElement (/builds/slave/test/gaia/node_modules/marionette-helper/index.js:139:19)
00:28:39     INFO -        at Object.Base.waitForElement (/builds/slave/test/gaia/apps/settings/test/marionette/app/base.js:56:31)
00:28:39     INFO -        at Object.KeyboardApp.goBackToSettingsApp (/builds/slave/test/gaia/apps/settings/test/marionette/app/keyboard_app.js:71:27)
00:28:39     INFO -        at Context.<anonymous> (/builds/slave/test/gaia/apps/settings/test/marionette/tests/keyboard_settings_test.js:24:17)
00:28:39     INFO -        at callFn (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:223:21)
00:28:39     INFO -        at Test.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:216:7)
00:28:39     INFO -        at Runner.runTest (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:373:10)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:451:12
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:298:14)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:308:7
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:246:23)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:270:7
00:28:39     INFO -        at done (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:185:5)
00:28:39     INFO -        at callFn (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:228:7)
00:28:39     INFO -        at Hook.Runnable.run (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:216:7)
00:28:39     INFO -        at next (/builds/slave/test/gaia/node_modules/mocha/lib/runner.js:258:10)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runner.js:270:7
00:28:39     INFO -        at done (/builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:185:5)
00:28:39     INFO -        at /builds/slave/test/gaia/node_modules/mocha/lib/runnable.js:199:9
00:28:39     INFO -        at Object.executeHook (/builds/slave/test/gaia/node_modules/marionette-client/lib/marionette/client.js:369:18)
00:28:39     INFO -        at process._tickCallback (node.js:415:13)

https://tbpl.mozilla.org/?rev=aa249d683122bbbf4371d5902a770beac451dd01&tree=Gaia-Try

You probably want to get this patch reviewed by a keyboard peer before re-landing.

	Yan Or Assignee
	Comment 17 • 10 years ago

Comment on attachment 8443246 [details]
Rebased pull request

Fixed marionette test.

Rudy, please review.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 18 • 10 years ago

Comment on attachment 8443246 [details]
Rebased pull request

I Cannot find the modified marionette test in the patch.
Wrong link?

	Yan Or Assignee
	Comment 19 • 10 years ago

Sorry, didn't realize the old PR has been closed.

	Rudy Lu [:rudyl] (inactive after 2015/7/3)
	Comment 20 • 10 years ago

Comment on attachment 8472368 [details]
New PR

It's ok.

This looks good to me, but I have 2 questions,
 1. The title "Keyboard Settings" would not be centered, and this is a known issue, do you think this could be addressed by the gaia-header component?

 2. The color the title is slightly changed, could we customize it to make it the same as the original color?

Thanks.

	Yan Or Assignee
	Comment 21 • 10 years ago

Rudy,

1. Yes, I've raised a separate issue about some header titles not centering.  This should be fixed in gaia-header.

2. We are doing a visual refresh of all the apps and this is the standardized color for settings related screens.

Thanks.

	Wilson Page [:wilsonpage]
	Comment 22 • 10 years ago

Comment on attachment 8472368 [details]
New PR

LANDED (master) https://github.com/mozilla-b2g/gaia/commit/e57ea93196e997336934d6e47323755c5bdd80e9