998847 - [Settings] refactor storage.js with AMD pattern

Status: RESOLVED FIXED

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

Description

[[e-mail]]

`storage.js` is lazy loaded in settings.js > root panel

We could use AMD pattern and mvvm to observe the storage result for root panel.

The result could be reused for  app storage and media storage panel

Comment 1

[[e-mail]]

root panel only

Comment 2

[[e-mail]]

I think I can't make it done in next week, postpone to s4

Comment 3

[[e-mail]]

Attachment: pull request redirect to github

the patch did:

1. moved storage.js into panels/root
2. make it pure AMD with AppStorage observer
3. move UMD dialog from index.html to root.html

I think the full patch could be done after MediaStorage refactor, but it's hard to estimate the MediaStorage refactor done date.

Comment 4

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

Comment on attachment 8438101 [details] [review]
pull request redirect to github

The changes look good to me, and I left some comments on github, please check it when you have time, Fred.

THanks ;)

Comment 5

[[e-mail]]

updated. found there's marionette test already, will update it as well

Comment 6

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

Comment on attachment 8438101 [details] [review]
pull request redirect to github

Thanks for the patch!

As r.js helps on merge scripts for us, I would prefer to have separate modules being responsible for each menu item, especially when the code for these items are pretty isolated.

And if there are handlers updating UI elements, make sure they are un-registered when leaving the panel. We can use a simple enabled property to do that, you may refer to keyboard related modules for details.

Comment 7

[[e-mail]]

Comment on attachment 8438101 [details] [review]
pull request redirect to github

comment addressed, add enabled function to register/un-register observer while enter/leaving the panel

Comment 8

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

Comment on attachment 8438101 [details] [review]
pull request redirect to github

As comment 6 suggested, please create separated modules for each menu item.

Comment 9

[[e-mail]]

found the USB storage and media storage have some connection between updateVolumeState.

Seems we have to refactor media storage first to make the panel separation clean.

Comment 10

[[e-mail]]

Attachment: PR2

per offline discussion with arthur, I separate the app storage menu module, but keep usb and media storage in same module. The media storage module will be separated until bug media storage refactor is done

root panel marionette test passed

Comment 11

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

Comment on attachment 8439046 [details] [review]
PR2

(In reply to Fred Lin [:gasolin] from comment #10)
> Created attachment 8439046 [details] [review]
> PR2
> 
> per offline discussion with arthur, I separate the app storage menu module,
> but keep usb and media storage in same module. The media storage module will
> be separated until bug media storage refactor is done
> 
> root panel marionette test passed

Ok got it !

Just left some feedbacks on Github and please ping me directly if you have any concern about comments, Fred.

Overall they all look good to me and you can set me f? again when you are ready. 

Thanks Fred.

Comment 12

[[e-mail]]

Comment on attachment 8439046 [details] [review]
PR2

Thanks EJ! I've fixed binding issue and move all private functions prefix with '_'. please kindly check it.

Comment 13

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

Comment on attachment 8439046 [details] [review]
PR2

The patch conflicts to gaia master. Could you help rebase it?

Comment 14

[[e-mail]]

Attachment: PR3

rebased the root panel. please review it again

Comment 15

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

Comment on attachment 8440554 [details] [review]
PR3

Only few nits on the patch, please check it on Github.

Basically, they all look good to me.

Comment 16

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

Comment on attachment 8440554 [details] [review]
PR3

Overall it looks good to me, please check my comments in github, thanks!

Comment 17

[[e-mail]]

Attachment: PR5

fix addressed issues and python UI tests, please kindly review it again, thanks!

Comment 18

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

Comment on attachment 8448519 [details] [review]
PR5

Thanks for the patch. Please check the comments regarding the naming and unit tests I left in github.

Comment 19

[[e-mail]]

Comment on attachment 8448519 [details] [review]
PR5

comment addressed, please kindly review it

Comment 20

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

Comment on attachment 8448519 [details] [review]
PR5

Looks good to me, thank you!

Comment 21

[[e-mail]]

merged to master https://github.com/mozilla-b2g/gaia/commit/8304cac69c74b0b5d25eca638c0e79113870bb39

thanks!