Closed Bug 984449 Opened 7 years ago Closed 7 years ago

Unify sim-picker elements code into shared

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: drs, Assigned: drs)

References

Details

Attachments

(1 file)

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.
Depends on: 947140
Blocks: b2g-dsds-1.4
Assignee: nobody → drs+bugzilla
Status: NEW → ASSIGNED
Attachment #8396057 - Flags: review?(yurenju.mozilla)
Attachment #8396057 - Flags: review?(francisco.jordano)
Attachment #8396057 - Flags: review?(felash)
Attachment #8396057 - Flags: review?(anthony)
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!
Attachment #8396057 - Flags: review?(francisco.jordano) → review+
Attachment #8396057 - Flags: review?(anthony) → review+
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
Attachment #8396057 - Flags: review?(yurenju.mozilla)
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.
Attachment #8396057 - Flags: review?(yurenju.mozilla)
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!
Attachment #8396057 - Flags: review?(yurenju.mozilla) → review+
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.
Attachment #8396057 - Flags: review?(felash) → review-
Comment on attachment 8396057 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/17554

Updated PR.
Attachment #8396057 - Flags: review- → review?(felash)
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
Attachment #8396057 - Flags: review?(felash) → review+
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?
Flags: needinfo?(yurenju.mozilla)
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.
root cause is maxBuffer for child_process.exec is exceeded. follow-up bug 992773 is filed.
Flags: needinfo?(yurenju.mozilla)
drs, it should work if rebase from master again.
(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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.