1162030 - [Messages][NG] Figure out how navigation works

Status: RESOLVED FIXED

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

Comment 1

[Oleg Zasypkin [:azasypkin]]

Not in the sprint, but likely will be moving in "out-of-sprint" mode.

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: github PR

not working WIP

Comment 5

[Julien Wajsberg [:julienw]]

Comment on attachment 8623785 [details] [review]
github PR

Hey Steve, Oleg,

I think this is ready for a first feedback pass !

Thanks :)

Comment 6

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8623785 [details] [review]
github PR

I think it's a good basis to iterate on. We can polish and improve Navigation progressively, especially we can simplify Navigation a lot once we split all panels and possibly move ReportView out of navigation subsystem and make it dialog instead.

It supports both NGA and old models, basically all we need :)

I haven't tested much, but in browser it works good so far!

Thanks!

Comment 8

[Steve Chung [:steveck]]

Comment on attachment 8623785 [details] [review]
github PR

Overall looks good and we can leave some known issues for follow up, thanks!

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8623785 [details] [review]
github PR

Hey folks,

I think this is ready for a review, at last !

I didn't handle all your comments but I think we can handle those in follow-ups.
Things that come to my mind:
* the report view animation should not be a panel animation, but should be a dialog animation.
* possibly use URL and URLSearchParams in Utils.params and Utils.url

Comment 10

[Oleg Zasypkin [:azasypkin]]

Hey Julien,

Sorry for the delay, I've almost reviewed your PR, but I need some more time to finish review and play with the patch on device. Will be the first thing I'll do on Thursday :)

Thanks!

Comment 11

[Oleg Zasypkin [:azasypkin]]

Hey,

I've left my last comments/questions at Github, mostly nits.

I've briefly tested on device and it looks good so far! However I noticed some Gecko graphics issues that probably have been made more visible with the new changes. So I'd prefer to keep r? flag for now and finish testing once Gecko is fixed.

Thanks!

Comment 12

[Julien Wajsberg [:julienw]]

Pushed a new version with comments fixed :) There are 2 commits because I've put the changes to navigateToComposer to a separate commit.

Comment 13

[Julien Wajsberg [:julienw]]

Pushed a version with integration tests fixed... It's actually important to wait for Drafts.request() before saving the draft in activity_handler.js.

So I think it's ready for a new review.

The gaia try for the new push won't have the unit tests properly run because of bug 1190180.

I also found a bug I'll file separately: in the "new" activity, if we find a contact for a phone number, we lose the body if it was specified.

Comment 14

[Julien Wajsberg [:julienw]]

Pushed a new version with (I think) the last issues we saw fixed. This works well on my device (although everything feels slow -- but I don't think this is my fault :D)

Comment 15

[Julien Wajsberg [:julienw]]

Everything was feeling slow... and it was partly my fault :) Now this is faster than master !

Just pushed a new version.

Comment 16

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8623785 [details] [review]
github PR

Looks good to me!

Just small nit at github and two issues that we're going to handle in follow-ups:
* "Cancel" button in "Discard unsent message" dialog doesn't work;
* We load Inbox before going to the notification conversation when app is run via notification click.

Great job!

Comment 17

[Steve Chung [:steveck]]

Comment on attachment 8623785 [details] [review]
github PR

Thanks for all the great works :)  It's really awesome and we can file some minor follow up later instead on blocking this for master.

Comment 18

[Julien Wajsberg [:julienw]]

landed in master: 68d369c2a2b0cd16db028f0c2e660107c52ce113

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #16)
> Comment on attachment 8623785 [details] [review]
> github PR
> 
> Looks good to me!
> 
> Just small nit at github

Fixed the nit.

> * "Cancel" button in "Discard unsent message" dialog doesn't work;

This was fixed.

> * We load Inbox before going to the notification conversation when app is
> run via notification click.

Filed bug 1192263.

> * the report view animation should not be a panel animation, but should be a dialog animation.

Filed Bug 1192267.

> * possibly use URL and URLSearchParams in Utils.params and Utils.url

Filed bug 1192266.



More issues I found while working on this bug:
* bug 1190275 - [Messages] Image attachments are not showing up properly when going to a subview and coming back
* bug 1190276 - [Messages] In a "new" activity we don't keep the body if the phone number has a conversation already