Closed
Bug 1175503
Opened 10 years ago
Closed 10 years ago
[Messages][NG] Get rid of direct ConversationView dependencies from Compose component
Categories
(Firefox OS Graveyard :: Gaia::SMS, defect)
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).
Comment 1•10 years ago
|
||
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.
Assignee | ||
Comment 2•10 years ago
|
||
(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.
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)
yeah, this makes me happier :)
Attachment #8624224 -
Flags: feedback?(felash) → feedback+
Updated•10 years ago
|
Blocks: sms-sprint-FxOS-S3
Whiteboard: [p=1]
Assignee | ||
Comment 8•10 years ago
|
||
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 9•10 years ago
|
||
Comment on attachment 8624224 [details] [review]
GitHub pull request URL (wip)
Let's do one more round :)
Attachment #8624224 -
Flags: review?(felash)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
Thanks!
Treeherder is green, landed.
Master: https://github.com/mozilla-b2g/gaia/commit/e8e047d02fd3528325a3b0cc05391cbdb2cbffa7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•