Closed Bug 1072856 Opened 7 years ago Closed 7 years ago

[Messages] Tapping on empty space above send and attach buttons should assimilate currently edited recipient

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 affected)

RESOLVED FIXED
2.2 S1 (5dec)
Tracking Status
b2g-v2.0 --- affected
b2g-v2.1 --- affected
b2g-v2.2 --- affected

People

(Reporter: azasypkin, Assigned: rishav_, Mentored)

Details

(Keywords: regression, Whiteboard: [sms-papercuts][lang=js][mentor-lang=ru])

Attachments

(1 file, 2 obsolete files)

Looks like it worked before, but regressed at some point.
Keywords: regression
Summary: [Messages] Tapping on empty space above send and attach button should assimilate currently edited recipient → [Messages] Tapping on empty space above send and attach buttons should assimilate currently edited recipient
Whiteboard: [sms-papercuts]
Oleg, would you be willing to mentor a new contributor through this?
Flags: needinfo?(azasypkin)
(In reply to Mike Hoye [:mhoye] from comment #1)
> Oleg, would you be willing to mentor a new contributor through this?
Yeah, it shouldn't be hard to fix. But there is a chance that we'll fix it before we find a volunteer :)
Mentor: azasypkin
Flags: needinfo?(azasypkin)
Whiteboard: [sms-papercuts] → [sms-papercuts][lang=js][mentor-lang=ru]
Assignee: nobody → rishav006
Status: NEW → ASSIGNED
Hi OLeg
Here is the PR . Please have a look into this.
Thanks
Attachment #8501811 - Flags: review?(azasypkin)
Comment on attachment 8501811 [details] [review]
apping on empty space above send and attach buttons should assimilate currently edited recipient.

Hey,

Thanks for the patch! In general it looks good, I just think that we need to move that code to Compose.js instead + few nits on GitHub.

Once you're ready please ask review from Julien or Steve as I won't be around for the next two weeks :)
Attachment #8501811 - Flags: review?(azasypkin)
Hi Juliew
Here is attached PR.
Thanks
Attachment #8501811 - Attachment is obsolete: true
Attachment #8504834 - Flags: review?(felash)
Comment on attachment 8504834 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient.

The current patch is fine, but there is one more case that doesn't work like it used to in previous Firefox OS version: we should assimilate when tapping the empty space in the middle of the screen (the space that is between the recipient panel and the composer input).

Can you please have a look to fix this as well? It will probably be in ThreadUI this time.

Thanks a lot !
Attachment #8504834 - Flags: review?(felash)
Comment on attachment 8504834 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient.

Hi Julien
I have the updated the patch. Have a look.
Thanks
Attachment #8504834 - Flags: review?(felash)
Comment on attachment 8504834 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient.

As I said, the code that manages the click on the container needs to be in ThreadUI, because ThreadUI is the component that logically contains this element.

Please request review once you're ready, thanks!
Attachment #8504834 - Flags: review?(felash)
Comment on attachment 8504834 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient.

Hi Julien
Hope this time it will be ok. :)
Here is the updated PR .
Thanks
Attachment #8504834 - Flags: review?(felash)
Comment on attachment 8504834 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient.

r=me
thanks !
Attachment #8504834 - Flags: review?(felash) → review+
master: 1926e9d063b024748e13eb0aa347f18ee57a6cb4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
This possibly made the event fluffing algorithm upset (I mean: it's more difficult to focus the composer now).

NI me to test this.
Flags: needinfo?(felash)
Flags: needinfo?(felash)
Flags: needinfo?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #12)
> This possibly made the event fluffing algorithm upset (I mean: it's more
> difficult to focus the composer now).
> 
> NI me to test this.

Looks like you were right: reverting this, I can much more easily focus on the composer text input field :(
Flags: needinfo?(felash)
Thanks for testing :)
I'd like to test with the patch in bug 946731 (hopefully it's not too much rotten)
Flags: needinfo?(felash)
reverted on master: 744f955addea27deab345fcffa67a0a3ca0a0a23

Sorry, I didn't have the time to work on bug 946731.
So I revert this, and we'll have to add specific listeners on the left and right parts instead of having a listener on the whole form like I suggested.

Sorry Rishav :(
Status: RESOLVED → REOPENED
Flags: needinfo?(felash)
Resolution: FIXED → ---
Okay No problem :)
I will look into this soon .

Thanks
Hi Julien
Have a look on the PR. I have implemented left and right side separately. for left side, created a new div element. Hope there won't be any regression now.
Thanks
Attachment #8504834 - Attachment is obsolete: true
Attachment #8528888 - Flags: review?(felash)
Comment on attachment 8528888 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient

Just some nits and I think that we'll be good.

Please add your new changes in a separate commit as it makes the next review easier :)

Hey Oleg, just NI you because I left a question for you on the pull request.
Flags: needinfo?(azasypkin)
Attachment #8528888 - Flags: review?(felash)
Comment on attachment 8528888 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient

Hi Julienw
Updated the patch.
Left comment on github about the ni? (oleg), as far as i checked.
Thanks
Attachment #8528888 - Flags: review?(felash)
(In reply to Julien Wajsberg [:julienw] from comment #18)
> Comment on attachment 8528888 [details] [review]
> Tapping on empty space above send and attach buttons should assimilate
> currently edited recipient
> 
> Hey Oleg, just NI you because I left a question for you on the pull request.

Replied on Github.
Flags: needinfo?(azasypkin)
Comment on attachment 8528888 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient

Hi julienw
Updated the PR and had the discussion with oleg on this. Have a look on this.
Thanks
Comment on attachment 8528888 [details] [review]
Tapping on empty space above send and attach buttons should assimilate currently edited recipient

r=me if you correct the latest small nits

When you've finished, you can squash your commits, push to the pull request, and ask "checkin-needed" :)

Thanks a lot !
Attachment #8528888 - Flags: review?(felash) → review+
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/39214fb22c203e8849aaa1c27b773eeb73212921
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S1 (5dec)
Mentor: felash
You need to log in before you can comment on or make changes to this bug.