1125708 - [settings] refactor homescreen panel with AMD pattern

Status: RESOLVED FIXED

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

Description

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

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

Comment 1

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Attachment: patch on master

Comment 2

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8554399 [details] [review]
patch on master

Hi Arthur, can you help me review this patch ? thanks :)

Comment 3

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8554399 [details] [review]
patch on master

Although the code works well, it would be better to do logic/UI separation.

A separate module can be created, which is an `Observable` with a property reporting the current icon layout and a function for setting the layout. The code in panel.js simply binds the UI element to the obserable. Let me know if any problems, thanks.

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Arhtur, this panel is quite simple enough and only have one select element, why we should use observable in this case ? I think this would be too much for this simple panel ?

Comment 5

[Arthur Chen [:arthurcc][inactive after 6/30]]

As the offline discussion, using observables helps separate the logic and UI. It is also easier to write unit tests for it.

Comment 6

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8554399 [details] [review]
patch on master

Arthur, thanks for providing the feedback about using observable. With this latest design, I think our codebase is quite cleaner than ever. Hope this makes sense to you !

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8554399 [details] [review]
patch on master

Per the offline discussion, let's add a function for setting the layout, thanks.

Comment 8

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8554399 [details] [review]
patch on master

Arthur, I just updated this patch based on our offline discussion, please help me check this when you have time !  Thanks :)

Comment 9

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8554399 [details] [review]
patch on master

r=me, thank you!

Comment 10

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Thanks Arthur, this patch was merged into Gaia/master : https://github.com/mozilla-b2g/gaia/commit/b253a9a68d1091dd4319a0ab1f26521a6da689c5