Closed Bug 1162030 Opened 5 years ago Closed 4 years ago

[Messages][NG] Figure out how navigation works

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [p(FxOS-S4)=2][p(FxOS-S3=3][p(2.2S13)=2][sms-sprint-2.2S14])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
steveck
: review+
azasypkin
: review+
steveck
: feedback+
azasypkin
: feedback+
Details | Review
No description provided.
Depends on: 1162028, 1162027
Assignee: nobody → felash
Summary: [Messages][New Gaia Architecture] use little-browser in views/index.html → [Messages][New Gaia Architecture] use gaia-navigator in views/index.html
Not in the sprint, but likely will be moving in "out-of-sprint" mode.
Whiteboard: [p=2] → [p=2][sms-sprint-2.2S14]
Summary: [Messages][New Gaia Architecture] use gaia-navigator in views/index.html → [Messages][New Gaia Architecture] figure out how navigation works
Duplicate of this bug: 1155537
Blocks: 1155534
No longer blocks: 1155509
Summary: [Messages][New Gaia Architecture] figure out how navigation works → [Messages][NG] Figure out how navigation works
Duplicate of this bug: 1162034
Attached file github PR
not working WIP
Whiteboard: [p=2][sms-sprint-2.2S14] → [p(FxOS-S3=3][p(2.2S13)=2][sms-sprint-2.2S14]
Comment on attachment 8623785 [details] [review]
github PR

Hey Steve, Oleg,

I think this is ready for a first feedback pass !

Thanks :)
Attachment #8623785 - Flags: feedback?(schung)
Attachment #8623785 - Flags: feedback?(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!
Attachment #8623785 - Flags: feedback?(azasypkin) → feedback+
Duplicate of this bug: 1122174
Comment on attachment 8623785 [details] [review]
github PR

Overall looks good and we can leave some known issues for follow up, thanks!
Attachment #8623785 - Flags: feedback?(schung) → feedback+
Depends on: 1082052
Depends on: 1185983
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
Attachment #8623785 - Flags: review?(schung)
Attachment #8623785 - Flags: review?(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!
Status: NEW → ASSIGNED
Blocks: 1188765
Whiteboard: [p(FxOS-S3=3][p(2.2S13)=2][sms-sprint-2.2S14] → [p(FxOS-S4)=2][p(FxOS-S3=3][p(2.2S13)=2][sms-sprint-2.2S14]
No longer blocks: 1188765
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!
Pushed a new version with comments fixed :) There are 2 commits because I've put the changes to navigateToComposer to a separate commit.
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.
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)
Everything was feeling slow... and it was partly my fault :) Now this is faster than master !

Just pushed a new version.
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!
Attachment #8623785 - Flags: review?(azasypkin) → review+
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.
Attachment #8623785 - Flags: review?(schung) → review+
landed in master: 68d369c2a2b0cd16db028f0c2e660107c52ce113
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Depends on: 1192263
Depends on: 1192266
Depends on: 1192267
(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
Depends on: 1192754
Depends on: 1193029
Depends on: 1192751
Depends on: 1199593
Depends on: 1203886
Duplicate of this bug: 1124624
Duplicate of this bug: 879851
Duplicate of this bug: 974763
You need to log in before you can comment on or make changes to this bug.