984449 - Unify sim-picker elements code into shared

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Doug Sherk (:drs) (inactive)]

Right now, we have sim-picker elements in SMS and twice in comms. We should unify them together in shared/, perhaps in a new shared/elements/ folder.

Comment 1

[Doug Sherk (:drs) (inactive)]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

Comment 2

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

Tried building, working good, also check contacts and working as expected.

Thanks a lot Dough!

Comment 3

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

Hi Doug, that's great but please add inteigration test for build system to check element file in zip.

set review flag to me if you have updated pull request.

FYI,
https://github.com/mozilla-b2g/gaia/blob/master/build/test/integration/build.test.js

Comment 4

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

(In reply to Yuren [:yurenju][:小朱] from comment #3)
> Comment on attachment 8396057 [details] [review]
> Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554
> 
> Hi Doug, that's great but please add inteigration test for build system to
> check element file in zip.
> 
> set review flag to me if you have updated pull request.
> 
> FYI,
> https://github.com/mozilla-b2g/gaia/blob/master/build/test/integration/build.
> test.js

I couldn't find any examples of other checks for files being there other than files like configs, json, etc., so I wasn't sure exactly how to structure this. I updated the PR.

Comment 5

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

integration test looks good, thank you!

Comment 6

[Julien Wajsberg [:julienw]]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

The patch is mostly fine, I put "r-" just because of others r+.

I want a test to check that the sim picker is correctly loaded and translated in ThreadUI.onBeforeEnter.

And you need to put "tabIndex=-1" back on the element in the sms app, because otherwise the keyboard does not hide.

Comment 7

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

Updated PR.

Comment 8

[Julien Wajsberg [:julienw]]

Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

r=me

thanks for the quick follow-up

Comment 9

[Doug Sherk (:drs) (inactive)]

After rebasing, I'm getting an error that I can't repro locally and that I have no clues as to what the cause could be:
https://travis-ci.org/mozilla-b2g/gaia/jobs/22121142

I messed around with the added test and based on the error messages when I play around with the path, I believe that it's being caused by this line:
https://github.com/DouglasSherk/gaia/commit/3c93ab8f24495a9273148571f6bc8ccc6beab65e#diff-81310329a99f792de16e7b35c5e697b3R511

I tried an experiment to fix it by switching from using the comms app to using the system app, but that didn't work:
https://github.com/DouglasSherk/gaia/commit/9f74fad9266f294fa1ff4a93bb0b6a4270ceed67

I'm wary of poking around more since I have to use Travis each time. Do you have any ideas, Yuren?

Comment 10

[Doug Sherk (:drs) (inactive)]

Oh, and just to clarify, I tested it and it's still working on my Fugu, and the tests pass locally, so the issue isn't that the changes (other than the added test) were broken by the rebase.

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

root cause is maxBuffer for child_process.exec is exceeded. follow-up bug 992773 is filed.

Comment 12

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

drs, it should work if rebase from master again.

Comment 13

[Doug Sherk (:drs) (inactive)]

(In reply to Yuren [:yurenju][:小朱] from comment #12)
> drs, it should work if rebase from master again.

Thank you Yuren, that fixed it!

https://github.com/mozilla-b2g/gaia/commit/80483b99338aaec71c6355acb68c6d45e531d967