[Messages][NG] Figure out how navigation works

RESOLVED FIXED

Status

Firefox OS
Gaia::SMS
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: julienw, Assigned: julienw)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
steveck
: review+
azasypkin
: review+
steveck
: feedback+
azasypkin
: feedback+
Details | Review | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

2 years ago
Depends on: 1162028, 1162027
(Assignee)

Updated

2 years ago
Assignee: nobody → felash
(Assignee)

Updated

2 years ago
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]
(Assignee)

Updated

2 years ago
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
Blocks: 1155507
No longer blocks: 1155534
Duplicate of this bug: 1162034
(Assignee)

Comment 4

2 years ago
Created attachment 8623785 [details] [review]
github PR

not working WIP
(Assignee)

Updated

2 years ago
Blocks: 1179700
Whiteboard: [p=2][sms-sprint-2.2S14] → [p(FxOS-S3=3][p(2.2S13)=2][sms-sprint-2.2S14]
(Assignee)

Comment 5

2 years ago
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+
(Assignee)

Updated

2 years ago
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+
(Assignee)

Updated

2 years ago
Depends on: 1082052
(Assignee)

Updated

2 years ago
Depends on: 1185983
(Assignee)

Comment 9

2 years ago
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

Updated

2 years ago
Blocks: 1188765
(Assignee)

Updated

2 years ago
Blocks: 1188768
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]
(Assignee)

Updated

2 years ago
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!
(Assignee)

Comment 12

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

Comment 13

2 years ago
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.
(Assignee)

Comment 14

2 years ago
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)
(Assignee)

Comment 15

2 years ago
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+
(Assignee)

Comment 18

2 years ago
landed in master: 68d369c2a2b0cd16db028f0c2e660107c52ce113
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Updated

2 years ago
Depends on: 1192263
(Assignee)

Updated

2 years ago
Depends on: 1192266
(Assignee)

Updated

2 years ago
Depends on: 1192267
(Assignee)

Comment 19

2 years ago
(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
(Assignee)

Updated

2 years ago
Depends on: 1192754
(Assignee)

Updated

2 years ago
Depends on: 1193029

Updated

2 years ago
Depends on: 1192751
(Assignee)

Updated

2 years ago
Depends on: 1199593
(Assignee)

Updated

2 years ago
Depends on: 1203886
Depends on: 1217859
(Assignee)

Updated

a year ago
Duplicate of this bug: 1124624
(Assignee)

Updated

a year ago
Duplicate of this bug: 879851
(Assignee)

Updated

a year ago
Duplicate of this bug: 974763
You need to log in before you can comment on or make changes to this bug.