Closed Bug 1087933 Opened 10 years ago Closed 8 years ago

Provide a way to switch from thread view to composer view

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: augustin.trancart, Assigned: augustin.trancart)

References

Details

Attachments

(2 files)

[Message] Thread menu v1.0.pdf
866.21 KB, application/pdf
Details
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996
46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review 
As of bug 918970, we now redirect to a thread (if it exists) from the contact app. 

We should provide a way to switch from a thread view to the composer view. The thread participant recipient must be in the new message recipient, and if the user was writing a message, it should be kept when switching to a new message.
Here it is. Should I NI Jenny to validate the description?
Flags: needinfo?(felash)
Hey Jenny,

this is a follow-up of your comment in Bug 918970 comment 55 :) This is also one of the things we discussed by mail. NI you so that you don't forget ;)
Flags: needinfo?(felash)
Depends on: 918970
Forgot to NI Jenny in comment 2 :)
Flags: needinfo?(jelee)
Hi there! I believe I'll be able to work on this in late November after 2.2 specs are delivered. Will get back to this, thanks!
Flags: needinfo?(jelee)
Hey Jenny! 

Did you have time to work on this? Thanks!
Flags: needinfo?(jelee)
Hello, see attached for first version of spec ;) Thanks!
Flags: needinfo?(jelee)
Just a notice to Augustin: don't pay attention to the design which uses next design of building blocks; just pay attention to the UX flow.
Hey Julien! I NI you because I have some questions about implementations. The patch is not ready yet. I need to write some unit tests, and maybe some marionette tests as well?
Answered on github :)
Note that the "feedback" flag is especially useful for such uses !

Marionette tests could be nice, especially because we'll eventually implement 2 different objects for the thread panel and the composer panel, and we don't want that this regresses.
Flags: needinfo?(felash)
Assignee: nobody → augustin.trancart
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Hey Julien,

It's been a long time, but here you are! My patch's updated \o/

For testing, I only added marionette tests. They seemed more relevant to me than unit testing there, but feel free to tell me if you see a good candidate for unit testing.

Thanks.
Attachment #8541259 - Flags: review?(felash)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

As we discussed, this needs to be rebased now that bug 1162030 landed. Sorry about that !

Fortunately now this part won't change much for hopefully a long time :)

I gave some directions on github, I hope this won't be too difficult now :)
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Hey Julien,

Here is the second part, now a lot more simpler with drafts. As the patch is really different than before, I thought more relevant not to push follow ups commit, but starting over instead. Please r?
Attachment #8541259 - Flags: review?(felash)
If that's fine to you, I'd like to wait for after the 2.5 branching before landing this. So it's quite lower priority for me during this week. I'll look at it for sure next week, but maybe this week if I have time :)
Completely fine, I didn't expect this to land in 2.5 :-)
Hey Julien, just a little reminder for you to review this, whenever you can :-)
Flags: needinfo?(felash)
yeah I know, crazy times :/ I know you're in holiday during this week and I'm sorry I was as busy :/
Flags: needinfo?(felash)
I left some comments already. I think you can already work on the integration tests, as they won't likely change even with any change I can ask on the main code. I'll give you more comments on the main code tomorrow.

Sorry again for the delay !
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Left comments on github :)

Please ping me in case you have questions !
Thanks again for your work, this is really good !  And sorry for the delay...
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Hey Julien, another try! Please r?
Attachment #8541259 - Flags: review?(felash)
Hey Matej,

can you have a look at the strings in the spec attachment 8533556 [details] ?

What I'd like to double check is the new string "Add recipient". It will be in the menu that's appearing when the user taps the top right button. And the goal is to move to a prefilled "new message" panel so that the user can add more recipients to this message.

That's why I would favor the string "Add more recipients" instead of "Add recipient". What do you think ? Maybe you'll have better ideas as well :)
Flags: needinfo?(matej)
Note that the current patch uses "Add Recipients" (with a final 's' and a capital letter at 'Recipient' :) ).
(In reply to Julien Wajsberg [:julienw] from comment #22)
> Hey Matej,
> 
> can you have a look at the strings in the spec attachment 8533556 [details] ?
> 
> What I'd like to double check is the new string "Add recipient". It will be
> in the menu that's appearing when the user taps the top right button. And
> the goal is to move to a prefilled "new message" panel so that the user can
> add more recipients to this message.
> 
> That's why I would favor the string "Add more recipients" instead of "Add
> recipient". What do you think ? Maybe you'll have better ideas as well :)

"Recipient" feels a bit cold and technical. Could we say something like "Add someone else" instead?

I actually don't mind it being singular, though. Saying "recipients" makes it feel like it's specifically for adding more than one person, but may lead someone to wonder how they just add one more.
Flags: needinfo?(matej)
"Add someone else" is maybe too generic though. Especially "Add", I'm not sure this convey enough meaning.

Another word could be "receiver", but it's really the same feeling.

More ideas:
* "Include more participants"
* "Include someone else"
Flags: needinfo?(matej)
(In reply to Julien Wajsberg [:julienw] from comment #25)
> "Add someone else" is maybe too generic though. Especially "Add", I'm not
> sure this convey enough meaning.
> 
> Another word could be "receiver", but it's really the same feeling.
> 
> More ideas:
> * "Include more participants"
> * "Include someone else"

I like the second one here. It's friendlier.
Flags: needinfo?(matej)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

added few more comments. We're closer than ever !

This is really cool !
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

hi Julien, here is another iteration, please r?
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Small nit in the test, I think we can do a cleaner job with the keyboard handling.

We're nearly there !
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Hey Julien please r? again :-)
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Sorry Augustin, it's orange :(

https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=8d832d45e4308c9319c0a56e539a4bbbfb5a2a6d&selectedJob=3389822

Moreover I wanted to mutualize things in one place, here I think it's better to use it in lib/keyboard...
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

Hey Julien,

Ooops! Corrected :-)

Also, I've tried to refit this as you wanted, but disclaimer: I'm not sure to have understood it properly :-)

Please r?
Attachment #8541259 - Flags: review?(felash)
Comment on attachment 8541259 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/26996

I left some small nits but r=me once you fix them and you check the SMS tests are green.

Thanks a lot, this is a great patch !
Hopefully it won't be the final one ;)
Attachment #8541259 - Flags: review?(felash) → review+
Don't forget to squash as well, of course
Yay \o/
Keywords: checkin-needed
Keywords: checkin-needed
master: 4f0e2a1a42a2d049b6fe8f4f095cdcdf0fd5465c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: