1155542 - [Messages][New Gaia Architecture] Centralizing the global components/styling into a folder

Status: RESOLVED FIXED

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

Description

[Steve Chung [:steveck]]

Since view separations will also reorganize HTML/CSS/JS into different folder, we'll also need to move global JS like utils.js or root.css into a "shared" folder in message.

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] steveck-chung:new-message-global > mozilla-b2g:master

Comment 2

[Steve Chung [:steveck]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Hi guys,

It's simply a rough idea about moving the "global"(because "shared" is for gaia shared component currently and I don't want them mixed up, but maybe you have better naming for this) assets(JS/CSS/others) into a subfolder since we might create subfolders for views in the future. I also move the desktoptop testing out of the js folder because it seems weird to keep it inside(Although we will remove this in the build time). Not sure if this structure is suitable for current master or should be better for new architecture(or you think we should keep this structure even in new gaia structure). It's just a WIP and path is not handled correctly, and any suggestion is welcome.

Comment 3

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Looks good to me, left some thoughts at GitHub. Generally speaking I'd rather move forward with smth like this sooner than later and we can refine this along the way to new structure.

Comment 4

[Julien Wajsberg [:julienw]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

feedback given on github

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Hi all,

The patch updated based on Oleg's new structure proposal, with the desktop-mock/app icon path changes. I tried to move the setup.js to shared but in vain :/ maybe I'm wrong bug I can't find a correct way to change setup.js load path.

Comment 6

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #5)
> Comment on attachment 8593885 [details] [review]
> [gaia] steveck-chung:new-message-global > mozilla-b2g:master
> 
> Hi all,
> 
> The patch updated based on Oleg's new structure proposal, with the
> desktop-mock/app icon path changes. I tried to move the setup.js to shared
> but in vain :/ maybe I'm wrong bug I can't find a correct way to change
> setup.js load path.

Yeah the path is hardcoded in [1].

[1] https://github.com/mozilla-b2g/gaia/blob/392ef2fafa48166378fb18e33da1ab4e04235e66/dev_apps/test-agent/common/test/agent_proxy.js#L26

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

I think we'll see better after bug 1156631.

Also I don't think we should move everything but only views-related files. For example time_headers.js should move, but threads.js shouldn't. Tell me what you think.

Comment 8

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Yeah, let's wait for bug 1156631 first maybe.

> Also I don't think we should move everything but only views-related files.
> For example time_headers.js should move, but threads.js shouldn't. 

I asked on Github, but if you mean that some of the files in sms/js/ will go to sms/services or sms/services/shared, then I yeah, I'd agree that we don't need to move them to views/shared/js. As for shared CSS and images - views/shared is the only good place IMO.

Comment 9

[Steve Chung [:steveck]]

I agree that we could set the backend/service-wise js into another folder(originally I didn't split these js files because they're not under the views, but now it's reasonable that some js shouldn't live in frontend side). I think maybe sms/services is enough and additional shared seem a little bit redundant. And about the utils.js, maybe I'll put it under views because most of functions are ui related, and split into ui/non-ui utils in the future?

Comment 10

[Julien Wajsberg [:julienw]]

Yeah, I agree with everyting you (Steve and Oleg) said here :)

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Another update after css separation finished. I only move threads/message_manager/drafts into services because that's what we confirmed firstly, but I'm not sure the necessasity of other js. Maybe we can decide here or in the bug for bridge library.

Comment 12

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

Looks good! Just few leftovers in unit tests and desktop mock (commented at GitHub).

I think we'll move the rest of files more precisely in the next patches.

Thanks!

Comment 13

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #12)
> Comment on attachment 8593885 [details] [review]
> [gaia] steveck-chung:new-message-global > mozilla-b2g:master
> 
> Looks good! Just few leftovers in unit tests and desktop mock (commented at
> GitHub).
> 
> I think we'll move the rest of files more precisely in the next patches.
> 
> Thanks!

Thanks for the catch! I found a weird issue about the XMLHttpRequest in unit test. xhr will return success with html blob even the given file is not existed. Since we don't check the actual file type(I don't think this is necessary anyway) our unit test will not be blocked by any error if assetHelper return a valid blob. I'll file a bug for it.

Comment 14

[Julien Wajsberg [:julienw]]

Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

I only have a small nit but otherwise this looks good, let's land this.

Comment 15

[Steve Chung [:steveck]]

Nit addressed, thanks!

Comment 16

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/f78ac92796e3aaf3000382886d7ce9be63aae4e7