Closed Bug 1175503 Opened 5 years ago Closed 5 years ago

[Messages][NG] Get rid of direct ConversationView dependencies from Compose component

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: azasypkin, Assigned: azasypkin)

References

Details

(Whiteboard: [p=1])

Attachments

(1 file)

In this bug we'd like to remove direct ConversationView dependencies from Compose component:

* ConversationView.draft;
* ConversationView.on('recipientschange', ...);
* ConversationView.recipients (+ Threads.active.participants).

This should make separation of Conversation and NewMessages views easier + narrow down ConversationView.draft usage (after this bug it will be used only inside ConversationView).
In my view, this was a goal of bug 1040144: by instanciating the compose view, we could pass the view as constructor (or factory) parameter.
(In reply to Julien Wajsberg [:julienw] from comment #1)
> In my view, this was a goal of bug 1040144: by instanciating the compose
> view, we could pass the view as constructor (or factory) parameter.

For this bug, I have a bit different idea in mind at the moment, will ask for feedback once I have something working :) Basically I want Composer to be entirely not aware of recipients.
OK, let me see once you have something :)

My original view was to have the same interface for recipients for both NewMessage view and ConversationView, so the composer could just call one polymorphic method and get done with it.
Here is the first WIP PR for what I had in mind.

Hey Julien,

It's still "very" WIP and some cases don't work yet, but it would be great to have your early feedback to validate this idea or come up with better one. I've left a bit lengthy explanation at GitHub to be on the same page :)

Thanks!
Attachment #8624224 - Flags: feedback?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

I didn't look really close but I don't mind using a modifier when we change the panel.

However I really think we need to provide value accessors to the Composer component, instead of the values themselves.
Attachment #8624224 - Flags: feedback?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Hey Julien,

Does it look better for you now?
Attachment #8624224 - Flags: feedback?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

yeah, this makes me happier :)
Attachment #8624224 - Flags: feedback?(felash) → feedback+
Whiteboard: [p=1]
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Hey Julien,

I've fixed your comments and added/updated unit tests for the affected parts. I could not find any issues during on-device-testing, so I need your fresh eyes on this :)

Could you please take a look?

Thanks!
Attachment #8624224 - Flags: review?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Let's do one more round :)
Attachment #8624224 - Flags: review?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

Added 3d commit :)

Please, tell if I missed something you wanted me to do!

Thanks!
Attachment #8624224 - Flags: review?(felash)
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)

r=me with the indentation nit in the test
Attachment #8624224 - Flags: review?(felash) → review+
Thanks!

Treeherder is green, landed.

Master: https://github.com/mozilla-b2g/gaia/commit/e8e047d02fd3528325a3b0cc05391cbdb2cbffa7
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.