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

RESOLVED FIXED

Status

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sms-sprint-2.2S11])

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → schung
Created attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master
(Assignee)

Comment 2

3 years ago
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.
Attachment #8593885 - Flags: feedback?(felash)
Attachment #8593885 - Flags: feedback?(azaaza)
(Assignee)

Updated

3 years ago
Attachment #8593885 - Flags: feedback?(azaaza) → feedback?(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.
Attachment #8593885 - Flags: feedback?(azasypkin) → feedback+
Comment on attachment 8593885 [details] [review]
[gaia] steveck-chung:new-message-global > mozilla-b2g:master

feedback given on github
Attachment #8593885 - Flags: feedback?(felash)
(Assignee)

Comment 5

3 years ago
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.
Attachment #8593885 - Flags: review?(felash)
Attachment #8593885 - Flags: review?(azasypkin)
(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
(Assignee)

Updated

3 years ago
Whiteboard: [sms-sprint-2.2S11]
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.
Attachment #8593885 - Flags: review?(felash)
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.
Attachment #8593885 - Flags: review?(azasypkin)
(Assignee)

Comment 9

3 years ago
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?
Yeah, I agree with everyting you (Steve and Oleg) said here :)
(Assignee)

Comment 11

3 years ago
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.
Attachment #8593885 - Flags: review?(felash)
Attachment #8593885 - Flags: review?(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!
Attachment #8593885 - Flags: review?(azasypkin) → review+
(Assignee)

Comment 13

3 years ago
(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 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.
Attachment #8593885 - Flags: review?(felash) → review+
(Assignee)

Comment 15

3 years ago
Nit addressed, thanks!
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.