Closed Bug 1156625 Opened 5 years ago Closed 5 years ago

[Messages] ThreadList&Thread separation: separate JS files

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.2 S11 (1may)

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

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

Attachments

(1 file)

In this bug we should move ThreadList and Thread specific JS files from :/js" folder to "views/thread-list/js" and "views/thread/js" respectively.
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

What do you guys think about such separation? 

Here I extract ThreadList and Thread-Composer from the common js folder, trying to keep it simple as possible.
Attachment #8595153 - Flags: feedback?(schung)
Attachment #8595153 - Flags: feedback?(felash)
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

Simple and clear, thanks!
BTW just an idea about the view naming: Do you think thread-list/thread is clear for the views? I think thread-list/message-list is more clear or even thread/message in short. Just an idea for the folder name first and maybe we can polish it while in further separation. It's because the original naming of view seems not very clear for the people not familiar with the code, even UX/QA are confused about the thread/thread list name sometimes.
Attachment #8595153 - Flags: feedback?(schung) → feedback+
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

looks about right but the unit tests needs to be moved accordingly to their app counterparts.
Attachment #8595153 - Flags: feedback?(felash) → feedback+
(In reply to Steve Chung [:steveck] from comment #3)
> Comment on attachment 8595153 [details] [review]
> [gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js >
> mozilla-b2g:master
> 
> Simple and clear, thanks!
> BTW just an idea about the view naming: Do you think thread-list/thread is
> clear for the views? I think thread-list/message-list is more clear or even
> thread/message in short. Just an idea for the folder name first and maybe we
> can polish it while in further separation. It's because the original naming
> of view seems not very clear for the people not familiar with the code, even
> UX/QA are confused about the thread/thread list name sometimes.

Both "thread-list/message-list" and current "thread-list/thread" look OK for me :) Maybe I just used to it, but I think we still need to keep this "-list" part. 

Let's ask 3rd opinion to be sure we all agree, Julien what do you prefer?

Votes:

thread-list/thread - Oleg+;
thread-list/message-list - Steve+; Oleg+;
thread/message - Steve+;
Flags: needinfo?(felash)
Another proposal:

inbox/conversation

(don't thank me :) )
Flags: needinfo?(felash)
Okay, I'll leave it as is for now :) Let's discuss together during review.
Jenny said she would prefer "inbox/conversation" if we're willing to change(And she is happy if we can do so :))
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

Hey Julien and Steve, I think PR is ready for the first review round :)

Thanks!
Attachment #8595153 - Flags: review?(schung)
Attachment #8595153 - Flags: review?(felash)
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

Hi Oleg, I left some comments on github but looks great overall, we can have a brand new naming for each view now \o/
Attachment #8595153 - Flags: review?(schung) → review+
Blocks: 1156631
This looks good, I just want to experiment a little on the phone but I don't have any comment so far.
Comment on attachment 8595153 [details] [review]
[gaia] azasypkin:bug-1156625-thread-list-and-thread-split-js > mozilla-b2g:master

ok, I found no issue, let's do this :)

I feel a little sad when I think to all my in-progress patch that I'll need to rebase ;)
Attachment #8595153 - Flags: review?(felash) → review+
Whiteboard: [sms-sprint-2.2S11]
Thanks for review guys!

> I feel a little sad when I think to all my in-progress patch that I'll need to rebase ;)

Yeah, you're not alone here :)
Keywords: checkin-needed
Target Milestone: --- → 2.2 S11 (1may)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.