Closed
Bug 1015295
Opened 10 years ago
Closed 10 years ago
[Keyboard] Update to use gaia-header
Categories
(Firefox OS Graveyard :: Gaia::Keyboard, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: yor, Assigned: yor)
References
Details
Attachments
(1 file, 2 obsolete files)
No description provided.
Blocks: gaia-header
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!
Attachment #8429820 -
Flags: review?(kgrandon) → review?(rlu)
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.
Attachment #8429820 -
Flags: review?(rlu)
Attachment #8429820 -
Flags: review+
Attachment #8429820 -
Flags: feedback?(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.
Updated•10 years ago
|
Attachment #8429820 -
Flags: feedback?(kgrandon) → feedback+
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
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?
Rudy, please back this out for now. We most likely won't land gaia-header in 2.0. Sorry about the confusion.
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8429820 -
Attachment is obsolete: true
Comment 11•10 years ago
|
||
yor: I'm not sure how to test this. When/how does the keyboard have a header?
Flags: needinfo?(yor)
Comment 12•10 years ago
|
||
Ah found it :) 'Settings' > 'Keyboard' > 'Built-In Keyboard'
Flags: needinfo?(yor)
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+.
Attachment #8443246 -
Flags: review?(wilsonpage) → feedback+
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.
Attachment #8443246 -
Flags: feedback+ → review?(wilsonpage)
Updated•10 years ago
|
Attachment #8443246 -
Flags: review?(wilsonpage) → review+
Comment 15•10 years ago
|
||
Comment on attachment 8443246 [details] Rebased pull request Landed on 'master' https://github.com/mozilla-b2g/gaia/commit/a7f25f36897b98dbb11c37f55cc1bbe57fcd326d
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.
Flags: needinfo?(yor)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8443246 [details]
Rebased pull request
Fixed marionette test.
Rudy, please review.
Attachment #8443246 -
Flags: review?(rlu)
Flags: needinfo?(yor)
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?
Attachment #8443246 -
Flags: review?(rlu)
Assignee | ||
Comment 19•10 years ago
|
||
Sorry, didn't realize the old PR has been closed.
Attachment #8443246 -
Attachment is obsolete: true
Attachment #8472368 -
Flags: review?(rlu)
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.
Attachment #8472368 -
Flags: review?(rlu) → review+
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.
Comment 22•10 years ago
|
||
Comment on attachment 8472368 [details] New PR LANDED (master) https://github.com/mozilla-b2g/gaia/commit/e57ea93196e997336934d6e47323755c5bdd80e9
Updated•10 years ago
|
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•