Closed Bug 973447 Opened 7 years ago Closed 7 years ago

[settings] refactor Language panel with AMD pattern

Categories

(Firefox OS Graveyard :: Gaia::Settings, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
1.4 S5 (11apr)

People

(Reporter: gasolin, Assigned: eragonj)

References

Details

Attachments

(1 file)

Overview Description:

Refactor Language panel with AMD pattern referring to
https://github.com/crh0716/gaia/tree/settings2_iterative

to make it modularize and more easier to maintain

Steps to Reproduce:
1) run make test-perf APP=settings
2) run make test-integration APP=settings


Expected Results:

pass all settings test and act the same as original implementation

Additional Information:
Attached file patch on master
Hi Fred & Arthur,

please give me some feedbacks so that I can make sure I am on the right way !

THanks :)
Attachment #8393983 - Flags: feedback?(gasolin)
Attachment #8393983 - Flags: feedback?(arthur.chen)
Comment on attachment 8393983 [details] [review]
patch on master

looks good but could be enhanced with modern requirejs pattern
Attachment #8393983 - Flags: feedback?(gasolin) → feedback+
Comment on attachment 8393983 [details] [review]
patch on master

Good work EJ!

As we are starting to come up concrete guidelines, I simply added some of my thoughts in github, and then we can discuss whether they are making sense or not. We need more practical experiences like this to make solid guidelines. :)
Attachment #8393983 - Flags: feedback?(arthur.chen) → feedback+
Thanks all, I would start to refactor based on your suggestions and talk with you guys with some thoughts. 

:)
Forgot to take the bug
Assignee: nobody → ejchen
Hi Fred & Arthur, 

how about this change !? :P
Flags: needinfo?(gasolin)
Flags: needinfo?(arthur.chen)
LGTM. Please check my comments in github.
Flags: needinfo?(arthur.chen)
Hi Arthur, 

I think this patch is almost done ! Can you help me check again :) ? 

If there is no big problem, I think we are ready to review :)
Flags: needinfo?(arthur.chen)
Comment on attachment 8393983 [details] [review]
patch on master

as comment in github
Flags: needinfo?(gasolin)
Comment on attachment 8393983 [details] [review]
patch on master

Arthur, I think this patch is ready for review :)
Attachment #8393983 - Flags: review?(arthur.chen)
Flags: needinfo?(arthur.chen)
Status: NEW → ASSIGNED
Comment on attachment 8393983 [details] [review]
patch on master

Please address my last nits (I promise) in github. Thanks! :]
Attachment #8393983 - Flags: review?(arthur.chen)
Blocks: 956210
Target Milestone: --- → 1.4 S5 (11apr)
Comment on attachment 8393983 [details] [review]
patch on master

Arthur, I think the PR looks nice now. I would set r? on you and plz help me review this patch when you have time (I think we are almost there !!! xD)
Attachment #8393983 - Flags: review?(arthur.chen)
Comment on attachment 8393983 [details] [review]
patch on master

r=me with the last nit addressed. Thanks!
Attachment #8393983 - Flags: review?(arthur.chen) → review+
(In reply to Arthur Chen [:arthurcc] from comment #13)
> Comment on attachment 8393983 [details] [review]
> patch on master
> 
> r=me with the last nit addressed. Thanks!

Thanks Arthur, I would rebase the code and make sure CI is passed. :)
Thanks Arthur and Fred, just merged the code into Gaia/master : 44a0bd975ea35879a013e82ef31a33058ae5a10e
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.