Closed Bug 1013296 Opened 6 years ago Closed 6 years ago

Compose. Change send button to an paper plane icon

Categories

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

x86
macOS
defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v2.0 verified, b2g-v2.1 verified)

VERIFIED FIXED
2.0 S5 (4july)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: vicky, Assigned: arnau)

References

Details

(Whiteboard: [p=1][not-part-of-initial-sprint])

Attachments

(5 files)

In order to match the email app send button, and to avoid spacing issues in different languages please replace the word "Send" for a paper plane icon.

This will also be very helpful when aligning character counter and MMS indicator.
blocking-b2g: --- → backlog
feature-b2g: --- → 2.0
Here are the assets.
Assignee: nobody → arnau
Target Milestone: --- → 2.0 S3 (6june)
How is implementation coming on this one? Sprint 3 ends Friday. Thanks!
Flags: needinfo?(arnau)
Working on it. 
The icon looks a bit larger than expected and need to fine-tune it with Pau.
I'll send a PR for review today.
Flags: needinfo?(arnau)
Comment on attachment 8433972 [details] [review]
patch in github

I left some comments on github, but I think we should wait for Bug 990537 first. Besides that, a major issue is indicator will disappear after this background image change. Please ask for review again after rebasing based on bug 990537 properly, thanks.
Attachment #8433972 - Flags: review?(schung)
Depends on: 990537
Blocks: 990537
No longer depends on: 990537
After thinking more, I now think this bug should land before bug 990537. I'll then do whatever needed to make DSDS work fine in bug 990537, so don't spend time on making DSDS work here.
Comment on attachment 8433972 [details] [review]
patch in github

Based on comment in https://github.com/mozilla-b2g/gaia/pull/19991/files#discussion_r13487895, I think we should wait at least for Bug 1008127 to have a proper place for send icon. If you want to land this patch first, maybe you could try to set the icon to send button background directly, but it seems violate the rule in building block...
Attachment #8433972 - Flags: review?(schung)
Depends on: 1008127
I was told by Yura from the Accessibility team that it's better to use aria-label instead of font-size or text-indent for adding the label for screen readers.
Could be a good idea to wait for bug 1015867 too. I'm changing how the send button is positioned (using a flex positioning instead of the current "postion: fixed").
Depends on: 1015867
Status: NEW → ASSIGNED
Target Milestone: 2.0 S3 (6june) → 2.0 S4 (20june)
Cool Julien, so I'll wait for your patch to land :)
About the accessibility thing:
is aria-label translated?
Because if it's not, my guess is having the text hidden with a l10n id would be a better option. WDYT?
Also adding Yura here for his input :)
Flags: needinfo?(yzenevich)
(In reply to Arnau March  [:arnau] from comment #10)
> About the accessibility thing:
> is aria-label translated?

Yes, aria-label is translated. See various bugs dependent on bug 893789, for example for the status bar etc., for the technique used.

> Also adding Yura here for his input :)

Taking the liberty of answering on his behalf instead. :)
Flags: needinfo?(yzenevich)
Blocks: sms-sprint-3
Whiteboard: [p=1]
(In reply to Marco Zehe (:MarcoZ) from comment #11)
> (In reply to Arnau March  [:arnau] from comment #10)
> > About the accessibility thing:
> > is aria-label translated?
> 
> Yes, aria-label is translated. See various bugs dependent on bug 893789, for
> example for the status bar etc., for the technique used.
> 
> > Also adding Yura here for his input :)
> 
> Taking the liberty of answering on his behalf instead. :)

Yes, what Marco said: 

{data-l10n-id}.ariaLanel = Your String

in the properties will do it.
Sorry, I meant {data-l10n-id}.ariaLabel
Target Milestone: 2.0 S4 (20june) → 2.0 S5 (4july)
Please update the status on this. Feature landing was two weeks ago and no more feature work is supposed to be in progress. We need to make a call on whether or not this is getting in.
Arnau is this patch ready for ui-review and r?
Flags: needinfo?(arnau)
Whiteboard: [p=1] → [p=1][not-part-of-initial-sprint]
No longer depends on: 1008127
(In reply to Stephany Wilkes from comment #14)
> Please update the status on this. Feature landing was two weeks ago and no
> more feature work is supposed to be in progress. We need to make a call on
> whether or not this is getting in.

Hi Stephany, Arnau couldn't land this patch because it depended on bug 1008127 (See comment 7), today this dependency was removed so tomorrow (today is local holiday in Barcelona) Arnau will ask for review and I hope that the patch will be merged.
Thanks!
feature-b2g: 2.0 → ---
Comment on attachment 8433972 [details] [review]
patch in github

Steve,
I have rebased my patch and changed the icon so it does not interfere with pseudo-elements in the button.
Attachment #8433972 - Flags: review?(schung)
Flags: needinfo?(arnau)
Comment on attachment 8433972 [details] [review]
patch in github

One small issue here:https://github.com/mozilla-b2g/gaia/pull/19991/files#discussion_r14232064 but I think overall is good. Please ask review again when you are ready, thanks!
Attachment #8433972 - Flags: review?(schung)
Attached image send.png
Attachment #8447084 - Flags: ui-review?(vpg)
Attachment #8447084 - Flags: ui-review?(vpg) → ui-review+
Comment on attachment 8433972 [details] [review]
patch in github

Looks great! r=me, thanks!
Attachment #8433972 - Flags: review?(schung) → review+
Travis is red because of other module failed. I restart the test but it should be fine if the test is still failed...
Since we already created the corresponding bugs for failed tests(bug 1030764 in everyting.me and bug 1032037 in music). It should be fine to land this patch.

In master: 345321f1d5d8a42a49e6818e9b0dd9a3258af2ef
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8433972 [details] [review]
patch in github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): new feature
[User impact] if declined: not following visual refresh targeted for 2.0
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]:
Attachment #8433972 - Flags: approval-gaia-v2.0?
Comment on attachment 8433972 [details] [review]
patch in github

Note to relman: This bug was a late request from visual design team. I'd really want this patch to be uplifted to v2.0 because:
* it will make it easier to work on bug 990537 (otherwise we'll need 2 separate patches for it)
* it will improve i18n (because we don't show any text now -- except for screenreader users)
* it's quite small (mostly CSS)
Attachment #8433972 - Flags: approval-gaia-v2.0? → approval-gaia-v2.0?(bbajaj)
Comment on attachment 8433972 [details] [review]
patch in github

Thanks for the additional info, the risk should be manageable given the changes. Approving for uplift.
Attachment #8433972 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
Tested and working
Hamachi
2.0
Gecko-7a31c32
Gaia-1bd6e89

Hamachi
2.1
Gecko-6777a8c
Gaia-a12ea45.
Status: RESOLVED → VERIFIED
This issue has been verified successfully on Flame2.0&2.1.
Reproducing rate: 0/5
See attachment: Verify_Flame_Send.mp4

Flame2.0 build version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141130000204
Version         32.0

Flame2.1 build version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141130001203
Version         34.0
Attached video Verify_Flame_Send.MP4
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.