Closed Bug 1178304 Opened 9 years ago Closed 9 years ago

Facebook share button in conversation window when waiting alone

Categories

(Hello (Loop) :: Client, defect, P1)

Product:
Hello (Loop)
Component:
Client
Type:
defect

Tracking

(firefox46 verified)

Status:
VERIFIED FIXED
Milestone:
mozilla46
Iteration:
46.2 - Jan 11
Tracking Flags:
Tracking Status
firefox46 --- verified

People

(Reporter: RT, Assigned: fcampo)

References

Details

(Whiteboard: [facebook])

User Story

As a desktop client user alone in a conversation, I want to share the conversation URL using Facebook easily.

Acceptance criteria:
* Facebook share button always available in the conversation window when waiting alone
* Clicking the Facebook share button opens the Facebook  share dialogue with the following pre-populated (using open graph tags: https://developers.facebook.com/docs/sharing/best-practices):
     - A 1200 x 630 pixels image promoting Firefox Hello (the visual is provided in https://bug1177949.bmoattachments.org/attachment.cgi?id=8635811)
     - Message: "Join me on a video call. Click the Firefox Hello link to connect now."
     - The title: "Join and create video calls free with Firefox Hello"
     - The site name: http://hello.firefox.com/
     - The shared URL: <Conversation URL>
     - The description: "Connect easily over video with anyone, anywhere. No downloads or registration."
* Clicks on the Facebook button trigger a Telemetry event (opt-out) to understand usage of the feature

Comments:
* Test URL with Facebook: https://developers.facebook.com/tools/debug/
* Facebook uses crawlers searching for open graph tags on web pages shared. These open graph tags can be used for site name, description, image and title it seems (https://developers.facebook.com/docs/sharing/best-practices)
* A simple link can be used as follows to open a Sharing dialogue: https://www.facebook.com/sharer/sharer.php?u=https://loop-webapp-demo.stage.mozaws.net/X0o7vhRvJAo#z5NrOR4BnBM0u7MUc4lRtA&title=Firefox Hello

Attachments

(6 files, 10 obsolete files)

Facebook button.png
(deleted), image/png
Details
wip video
(deleted), video/webm
Details
v1 screenshot
(deleted), image/png
Details
UI-10pt-8pt
(deleted), image/png
sevaan
: ui-review+
Details
Facebook share button in conversation when waiting alone
(deleted), patch
fcampo
: review+
Details | Diff | Splinter Review
Final look
(deleted), image/png
Details
No description provided.
User Story: (updated)
Flags: firefox-backlog+
Attached image Facebook button.png (deleted) —
The proposed UX implies that we remove the shareplane button. Fabio is that OK given some of the Fx39 comms on shareplane (not sure if it went out yet but perhaps we stop communicating on shareplane given it will eventually be removed from Firefox)? Sevaan is that OK given timelines to replace shareplane and wider UX share plans? (want to make sure the timing is right to remove shareplane now)
Flags: needinfo?(sfranks)
Flags: needinfo?(frios)
It's okay to remove the shareplane. The replacement sharing mechanism is currently still in the design stage.
Flags: needinfo?(sfranks)
Blocks: 1179164
User Story: (updated)
User Story: (updated)
RT says we can use the mechanism in the user story and later replace if Moz implements own. would take before refresh unless it doesn't make sense to.
Rank: 24
Priority: -- → P2
Whiteboard: [facebook]
(In reply to Sevaan Franks [:sevaan] from comment #3) > It's okay to remove the shareplane. The replacement sharing mechanism is > currently still in the design stage. I spoke with Chad and Madhava prior to the release and while there's been thinking about removing shareplane, we had not made any movements towards that change and it didn't sound like that was happening by 42. That is why we proceeded with comms. Also, the discussion is not about removing, it's more about replacing it with something else. The sharing aspect isn't going away. We should not remove it. The reasons are that it's still in Firefox (should be timed with that change), we don't have data on usage, and of course... we just announced it.
No longer blocks: 1179164
User Story: (updated)
Flags: needinfo?(frios)
Priority: P2 → --
Whiteboard: [facebook]
Priority: -- → P2
Whiteboard: [facebook]
Depends on: 1177949
User Story: (updated)
User Story: (updated)
Rank: 24
Priority: P2 → P1
Depends on: 1179164
Rank: 18
Amended US with reference to visual asset to be used.
User Story: (updated)
Sevaan, do you suggest we use a pop-up or a dialogue anchored to the Hello button for this?
I feel like the panel might be too large to anchor in a dialogue box. I think a pop-up is probably the best way forward for the moment.
need at least the invitation overlay completed to do this. bug 1184924
Depends on: 1184924
Rank: 18 → 20
Priority: P1 → P2
An initial look at this bug seems to indicate most of the heavy lifting is done in the standalone UI. The invitation overlay on the desktop conversation view would need a new event/link to Facebook, but Facebook's crawler will be hitting the standalone page.
Assignee: nobody → edilee
Attached video wip video (deleted) —
Here's a video of clicking the facebook invite button and the resulting sharer page. > - Message: "Join me on a video call. Click the Firefox Hello link to > connect now." Facebook doesn't allow pre-populated messages saying they only want organic user content. > - The title: "Join and create video calls free with Firefox Hello" > - The description: "Connect easily over video with anyone, anywhere. No > downloads or registration." Should these be localized or english is fine initially? > - The site name: http://hello.firefox.com/ Facebook says this needs to be a name not a url/domain. Their example was IMDb instead of imdb.com.
Attached patch wip (obsolete) (deleted) — — Splinter Review
Attachment #8671601 - Flags: feedback?
Comment on attachment 8671601 [details] [diff] [review] wip >+++ b/browser/components/loop/standalone/content/index.html > <head> >+ <meta property="og:description" content="Connect easily over video with anyone, anywhere. No downloads or registration." /> >+ <meta property="og:image" content="https://hello.firefox.com/img/invitation.png" /> >+ <meta property="og:image:height" content="630" /> >+ <meta property="og:image:width" content="1200" /> >+ <meta property="og:site_name" content="Firefox Hello" /> >+ <meta property="og:title" content="Join and create video calls free with Firefox Hello" /> After much trial an error trying to figure out what gets Facebook's scraper to immediately show an image on share (instead of on the 3rd share), it needs the meta tags static in the html with the image value being a full url (not relative). Should we just hardcode this in english for now given the description/title strings? Similarly, should we hardcode the host for the image?
Attachment #8671601 - Flags: feedback? → feedback?(standard8)
The hope is that the Facebook button can be uplifted with 43 - since these string are coded on the standalone client it sounds like we have until mid December to complete L10N so release users get localized strings? Also please note I'm discussing an update of these string (and also the e-mail copy strings) with marketing so that it fits better the sharing value proposition (as opposed to video calling) - hopefully very soon we'll get updated string to come with FF44 (will raise a bug once i get all details).
Some specific instructions on localizing the Facebook strings: https://developers.facebook.com/blog/post/605/ (under "Translating Objects"), also in http://stackoverflow.com/questions/20827882/in-open-graph-markup-whats-the-use-of-oglocalealternate-without-the-locati A brief summary: we need tags like <meta property="og:locale:alternate" content="es_ES"> and then Facebook will fetch https://hello.firefox.com/CODE?fb_locale=es_ES and then we localize the og: properties for that resource. My suggestion then is that we'd build separate pages for each locale, and use some configuration (maybe Nginx) to route these Facebook requests to, say, content/index.html.es_ES (which is some built version of content/index.html with localized substitutions).
I think "Join and create video calls free with Firefox Hello" doesn't properly express what the page is. Something like "Join me in a free video call with Firefox Hello" explains that this isn't a tool, but an actual contact method the person posting the URL is offering. The description might make sense as: "Firefox Hello allows you to connect easily over video with anyone, anywhere. No downloads or registration." – adding the product name to make it clear what is being described.
Regarding strings: I would suggest that we work up a system that allows us to use the L10n infra that we have now, rather than spinning up something of our own. Now we have a build step, there's various ways we could do this (FxA has one way, though I don't like it) which could also mean one less file for locales to download - we should take another look around and see what we can do. That will be more work, and hence we should split it off to another bug. Would it be reasonable to land a first version of this button with English only for the facebook page background - for testing in nighty/aurora/beta - with the aim of fixing the L10n of the standalone before we release?
For the full l10n fix, what language should the shared link be? Of the link sender or the link clicker? If the link sender, the url would need to tell the server the language so when Facebook crawls the page, it gets the right text. If the link clicker, we'll need the og:locale:alternate and have Facebook crawl the various versions of the page with ?fb_locale.
If we do the link clicker (i.e., og:locale:alternate) then the link will be shown in the native language of the person reading the Facebook post, which seems like the better option.
Ah I missed that bit. We definitely want to do this from the link clicker perspective. We can generate og:locale:alternate quite easily - we can use the script we have for importing locales to insert those. The harder bit will still be the index.html files themselves which we should think about more.
Comment on attachment 8671601 [details] [diff] [review] wip Review of attachment 8671601 [details] [diff] [review]: ----------------------------------------------------------------- We'll probably need to get the histogram change verified by one of the privacy folks when we do this, but seeing as its a simple addition, I don't see an issue with it. ::: browser/components/loop/content/js/roomStore.js @@ +363,5 @@ > + * > + * @param {sharedActions.FacebookShareRoomUrl} actionData The action data. > + */ > + facebookShareRoomUrl: function(actionData) { > + this._mozLoop.openURL("https://www.facebook.com/sharer/sharer.php?u=" + It might be nice to split this url out to a pref in case it changes - easier for patching.
Attachment #8671601 - Flags: feedback?(standard8) → feedback+
Comment on attachment 8671601 [details] [diff] [review] wip (In reply to Mark Banner (:standard8) from comment #22) > We'll probably need to get the histogram change verified by one of the > privacy folks when we do this, but seeing as its a simple addition, I don't > see an issue with it. Who do we request this from? I see various p?ally for other telemetry probe changes.. ally, this reuses the LOOP_SHARING_ROOM_URL probe by taking an unused histogram value (4) for use to count how many times the user clicks on a facebook share button from Hello's invitation view (see attachment 8671577 [details]). It doesn't necessarily mean the user has facebook or is logged in or shared. Just that the button was clicked.
Attachment #8671601 - Flags: feedback?(ally)
Comment on attachment 8671601 [details] [diff] [review] wip Review of attachment 8671601 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for contacting us. That's a reasonable extension of an existing probe. Should neither damage your telemetry data graphs nor increase risk. p=ally I'll be on pto soon, so if you need help while I'm out, :vladan is also a data collection steward and can also do those reviews if you need further help.
Attachment #8671601 - Flags: feedback?(ally) → review+
Attached patch v1 (obsolete) (deleted) — — Splinter Review
Attachment #8674138 - Flags: review?(standard8)
Attached image v1 screenshot (deleted) —
Moved the facebook button to the right to match the new web sharing mocks.
Attachment #8671601 - Attachment is obsolete: true
Comment on attachment 8674138 [details] [diff] [review] v1 Review of attachment 8674138 [details] [diff] [review]: ----------------------------------------------------------------- Passing to Dan to ajdust my review load a bit.
Attachment #8674138 - Flags: review?(standard8) → review?(dmose)
Comment on attachment 8674138 [details] [diff] [review] v1 Removing code review flag for now. There are some user-experience issues with the current setup that we need to work through first. Ed will be updating the bug...
Attachment #8674138 - Flags: review?(dmose)
dmose and I have several questions about the user story and expected UX: - Where did the sharer.php url come from? It was once deprecated but brought back, but there isn't much documentation about this endpoint. - "share" on Facebook defaults to posting on one's timeline, but the interface does allow the sharer to switch to someone else's timeline or send as private message. Is it okay that the default is the timeline? - "send" on Facebook is for sending a private message / email / to a group. There's a send dialog endpoint but it requires an app_id https://developers.facebook.com/docs/sharing/reference/send-dialog Do we want to use send instead of share? - Do we have a Facebook app_id to build as part of Firefox (probably packaged as a pref?)? - In all of the dialogs, the submit button is in the bottom right corner, which gets covered up by any open chat windows even when the chat is minimized. Should we open the share/send dialog in a separate window? - Facebook provides a FB.ui API that knows how to open dialogs, but this needs to run in a web page. Would it be okay to have the invite button open a tab that loads the FB.ui API that opens a dialog window and closes the tab? I'm thinking of this API call: FB.ui({ method: 'send', link: 'https://loop-webapp-demo.stage.mozaws.net/X0o7vhRvJAo#z5NrOR4BnBM0u7MUc4lRtA', }); Example links for comparison: sharer.php: https://www.facebook.com/sharer/sharer.php?u=https%3A%2F%2Floop-webapp-demo.stage.mozaws.net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA share dialog: https://www.facebook.com/dialog/share?app_id=145634995501895&display=popup&href=https%3A%2F%2Floop-webapp-demo.stage.mozaws.net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA&redirect_uri=https%3A%2F%2Fdevelopers.facebook.com%2Ftools%2Fexplorer send dialog: https://www.facebook.com/dialog/send?app_id=145634995501895&display=popup&link=https%3A%2F%2Floop-webapp-demo.stage.mozaws.net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA&redirect_uri=https%3A%2F%2Fdevelopers.facebook.com%2Ftools%2Fexplorer
Flags: needinfo?(sfranks)
Flags: needinfo?(rtestard)
(In reply to Ed Lee :Mardak from comment #29) > dmose and I have several questions about the user story and expected UX: > > - Where did the sharer.php url come from? It was once deprecated but brought > back, but there isn't much documentation about this endpoint. This was just an example of what the implementation could be like. If there is a better/easier to achieve this experience there are no issue with not using this. > - "share" on Facebook defaults to posting on one's timeline, but the > interface does allow the sharer to switch to someone else's timeline or send > as private message. Is it okay that the default is the timeline? If we can default to "private message" then it is ideal. > - "send" on Facebook is for sending a private message / email / to a group. > There's a send dialog endpoint but it requires an app_id > https://developers.facebook.com/docs/sharing/reference/send-dialog Do we > want to use send instead of share? The intended experience is to share the Hello URL, ideally through a private message. If "Send" allows providing that experience then great although I am not sure what the dependency on requiring an app_id means? > - Do we have a Facebook app_id to build as part of Firefox (probably > packaged as a pref?)? I NI Shane who may know. > - In all of the dialogs, the submit button is in the bottom right corner, > which gets covered up by any open chat windows even when the chat is > minimized. Should we open the share/send dialog in a separate window? Sevaan in Comment 9 was suggesting a pop-up window. > - Facebook provides a FB.ui API that knows how to open dialogs, but this > needs to run in a web page. Would it be okay to have the invite button open > a tab that loads the FB.ui API that opens a dialog window and closes the tab? > > I'm thinking of this API call: > > FB.ui({ > method: 'send', > link: > 'https://loop-webapp-demo.stage.mozaws.net/ > X0o7vhRvJAo#z5NrOR4BnBM0u7MUc4lRtA', > }); > > Example links for comparison: > > sharer.php: > https://www.facebook.com/sharer/sharer.php?u=https%3A%2F%2Floop-webapp-demo. > stage.mozaws.net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA > > share dialog: > https://www.facebook.com/dialog/ > share?app_id=145634995501895&display=popup&href=https%3A%2F%2Floop-webapp- > demo.stage.mozaws. > net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA&redirect_uri=https%3A%2F%2Fdevelop > ers.facebook.com%2Ftools%2Fexplorer > > send dialog: > https://www.facebook.com/dialog/ > send?app_id=145634995501895&display=popup&link=https%3A%2F%2Floop-webapp- > demo.stage.mozaws. > net%2FX0o7vhRvJAo%23z5NrOR4BnBM0u7MUc4lRtA&redirect_uri=https%3A%2F%2Fdevelop > ers.facebook.com%2Ftools%2Fexplorer
Flags: needinfo?(rtestard) → needinfo?(mixedpuppy)
Flags: needinfo?(sfranks)
:markh is also likely to have context about the facebook app id and available apis; Mark, do you have any thoughts here?
Flags: needinfo?(markh)
We do not use an app_id or any facebook APIs in socialapi and I have not seen any app_id in firefox. Share uses the sharer.php endpoint (as does much of the web, e.g. youtube) but it has no configurable parameters (e.g. to change the default posting type) that I have found.
Flags: needinfo?(mixedpuppy)
Looks like youtube has switched to the new share endpoint, e.g., https://www.facebook.com/dialog/share?app_id=87741124305&href=https%3A//www.youtube.com/watch%3Fv%3DTEPq4xFGzG0%26feature%3Dshare&display=popup&redirect_uri=https://www.youtube.com/facebook_redirect These endpoints want a redirect_uri to send the user after posting: https://www.youtube.com/facebook_redirect Looks like a common pattern to just have the popup close itself: <script>window.open('', '_self', ''); window.close();</script>
huh, maybe I need to look into changing that for share as well. If an app_id is setup for hello, make it a generic "Firefox" app_id so I can also use it.
Shane knows more about this than I do, and he's already on top of things...
Flags: needinfo?(markh)
Note that the final text for this (suitable for putting in a properties file) is in the user story of bug 1213906 and can be folded in here.
need to sort out localization and also sharing as message versus on timeline.
Assignee: edilee → nobody
Sevaan, Pau, To change this to default to share to message rather than the timeline (see video) will be a lot of work. question is should we implement as is and then improve? or hold until 46 or later to try for exact experience. this is kindof what facebook sharing always looks like - but not ideal.... talk to Dan for talk-offs
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
actually - talk to RT :)
(In reply to :shell escalante from comment #38) > Sevaan, Pau, > > To change this to default to share to message rather than the timeline (see > video) will be a lot of work. question is should we implement as is and > then improve? or hold until 46 or later to try for exact experience. this > is kindof what facebook sharing always looks like - but not ideal.... I think it's okay to implement and improve, but let's just set our expectations that it's probably not going to be a super-effective mechanism of sharing until the correct experience is there.
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
Agreed, let's implement and monitor how users use it (Telemetry and impact on clicker traffic originating from Facebook will help measure this). This is set with the right priority right now and my understanding is that the implementation has been done already so is this just a question of landing this in Nightly?
Flags: needinfo?(dmose)
(In reply to Romain Testard [:RT] from comment #41) > This is set with the right priority right now and my understanding is that > the implementation has been done already so is this just a question of > landing this in Nightly? I'll leave Dan to answer the main question. I'm slightly concerned about landing this without a defined technical path forward for localisation of the text that's in the standalone page. I think it'll be a couple of weeks before we can start to think about that. If we do land this then we need to commit to fixing the localisation issues before this ships - as a blocker that we don't back down on. I would say this needs localisation fixed early in the 46 cycle so we have time for deployment, localisation and testing.
It needs an in-depth review (it hasn't been reviewed yet), addressing any fallout that comes from that, and perhaps updating to account for e10s changes. So it's definitely more than just landing.
Flags: needinfo?(dmose)
taking this while Ed is away
Assignee: nobody → fernando.campo
Status: NEW → ASSIGNED
Attached image UI-after-patch (obsolete) (deleted) —
Attachment #8692471 - Flags: ui-review?(sfranks)
Attachment #8692471 - Flags: ui-review?(b.pmm)
Attached patch WIP - Facebook share button (obsolete) (deleted) — — Splinter Review
I've updated Ed's patch after RemotePageManager landed, and tested it (visuals on attachment 8692471 [details]. After all the previous conversation, it seems that we want to open the sharing in a popup window, which is not possible (AFAIK) with the current implementation. - Using the sharer instead of the send dialog (which requires app_id) doesn't allow us to open a popup, so we use - MozLoopApi.OpenURL, which only gives us access to .openUILinkIn(url, "tab") As the app_id has been decided to be left for a followup, another approach would be to create a more generical LoopAPI.OpenURL and LoopService.openURL so we can choose where to open (options are "current", "tab", "tabshifted", "window", "save"), but my testing < window.open(url) and Services.wm.getMostRecentWindow("navigator:browser").openUILinkIn(url, "tab") > on this were unsuccesful (probably need more changes I'm not aware of) Or, just land this as it is, keep the followup focused on using FB API.
Attachment #8674138 - Attachment is obsolete: true
Attachment #8692477 - Flags: review?(dmose)
...and because I'm still sleeping I added a wrong patch
Attachment #8692477 - Attachment is obsolete: true
Attachment #8692477 - Flags: review?(dmose)
Attachment #8692478 - Flags: review?(dmose)
Comment on attachment 8692471 [details] UI-after-patch Font size for the label should be 9pt. Also, I believe the original mockups have the string just being "Facebook" (http://i.sevaan.com/image/0Z0I3c3y0w15). "Share on Facebook" is a little too long and probably translates even longer.
Attachment #8692471 - Flags: ui-review?(sfranks)
Attachment #8692471 - Flags: ui-review?(b.pmm)
Attachment #8692471 - Flags: ui-review-
(In reply to Sevaan Franks [:sevaan] from comment #48) > Comment on attachment 8692471 [details] > UI-after-patch > > Font size for the label should be 9pt. It is already 9pt. You want it smaller, or prefer the text 'Invite a friend to join you' bigger? > Also, I believe the original mockups have the string just being "Facebook" > (http://i.sevaan.com/image/0Z0I3c3y0w15). "Share on Facebook" is a little > too long and probably translates even longer. Changing it Should I change the icon to the left as the mockup shows?
Flags: needinfo?(sfranks)
> It is already 9pt. You want it smaller, or prefer the text 'Invite a friend > to join you' bigger? Let's make the Invite a Friend text bigger then Try it at 12 and see how it looks. > Should I change the icon to the left as the mockup shows? No, the order you have is fine.
Flags: needinfo?(sfranks)
Attached image diff_size.png (obsolete) (deleted) —
I tested with a few, personally think that 12pt is too big, but asking for confirmation.
Attachment #8692932 - Flags: ui-review?(sfranks)
Let's do it at 10pt and make the labels 8pt instead of 9. Thanks for the comparison, it really helped.
Attachment #8692932 - Flags: ui-review?(sfranks) → ui-review-
Added :sevaan's comments and updated code after add-on landing
Attachment #8692478 - Attachment is obsolete: true
Attachment #8693619 - Attachment is obsolete: true
Attachment #8692478 - Flags: review?(dmose)
Attachment #8693628 - Flags: review?(dmose)
Iteration: --- → 45.3 - Dec 14
Attached image UI-10pt-8pt (deleted) —
Changes done
Attachment #8692471 - Attachment is obsolete: true
Attachment #8692932 - Attachment is obsolete: true
Attachment #8693632 - Flags: ui-review?(sfranks)
Attachment #8693632 - Flags: ui-review?(sfranks) → ui-review+
Rank: 20 → 19
Priority: P2 → P1
Comment on attachment 8693628 [details] [diff] [review] Facebook share button in conversation when waiting alone Review of attachment 8693628 [details] [diff] [review]: ----------------------------------------------------------------- Looking good; comments inline. ::: browser/extensions/loop/content/panels/js/roomStore.js @@ +368,5 @@ > + var from = actionData.from; > + var bucket = this._constants.SHARING_ROOM_URL["FACEBOOK_FROM_" + from.toUpperCase()]; > + if (typeof bucket === "undefined") { > + console.error("No URL sharing type bucket found for '" + from + "'"); > + return; Even if we can't log it in telemetry, don't we still want to notify the UI tour, instead of returning early? ::: browser/extensions/loop/content/panels/js/roomViews.jsx @@ +246,2 @@ > onEditContextClose: React.PropTypes.func, > + onAddContextClick: React.PropTypes.func, I'm not sure why the order got flipped here, but eslint doesn't like it, since it's no longer alphabetical. Please change this back. ::: browser/extensions/loop/content/shared/css/conversation.css @@ +236,5 @@ > left: -10rem; > margin: .5rem 0 0; > position: absolute; > right: -10rem; > + font-size: 1.1rem; /* 8pt */ The point of putting something in rems in general is so that its absolute size can vary as a function of root element's font size. Meaning that if that changes size, this comment will no longer be true. I'd propose removing this comment and the other one like it, unless there's some reason we'd want to preserve 8pt absolute height if we changed the root em. In which case, we should just use absolute units here and include a comment explaining why. I suspect we want the former and not the latter. But perhaps I'm missing something? ::: browser/extensions/loop/content/shared/js/actions.js @@ +365,5 @@ > + * Share a room url with Facebook. > + * XXX: should move to some roomActions module - refs bug 1079284 > + */ > + FacebookShareRoomUrl: Action.define("facebookShareRoomUrl", { > + from: String, Please add a comment here explaining what the range of "from" is and what it's used for. ::: browser/extensions/loop/standalone/content/index.html @@ +9,5 @@ > <meta name="default_locale" content="en-US" /> > <meta name="referrer" content="origin" /> > > + <meta property="og:description" content="Connect easily over video with anyone, anywhere. No downloads or registration." /> > + <meta property="og:image" content="https://hello.firefox.com/img/invitation.png" /> Please attach invitation.png (linked to from the User Story in this bug) to the next iteration of this patch. @@ +13,5 @@ > + <meta property="og:image" content="https://hello.firefox.com/img/invitation.png" /> > + <meta property="og:image:height" content="630" /> > + <meta property="og:image:width" content="1200" /> > + <meta property="og:site_name" content="Firefox Hello" /> > + <meta property="og:title" content="Join and create video calls free with Firefox Hello" /> This verbiage looks like it belongs to the old user journey. Sevaan/matej, do we have new verbiage here? ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +36,5 @@ > ## an iconic button for the invite view. > invite_copy_link_button=Copy Link > invite_copied_link_button=Copied! > invite_email_link_button=Email Link > +invite_facebook_button2=Facebook Since the semantics of the string have changed, please bump this to invite_facebook_button3 so that the localizers will know to re-translate this. ::: toolkit/components/telemetry/Histograms.json @@ +8506,5 @@ > "expires_in_version": "46", > "kind": "enumerated", > "n_values": 8, > "releaseChannelCollection": "opt-out", > + "description": "Number of times a room URL is shared (0=COPY_FROM_PANEL, 1=COPY_FROM_CONVERSATION, 2=EMAIL_FROM_CALLFAILED, 3=EMAIL_FROM_CONVERSATION, 4=FACEBOOK_FROM_CONVERSATION)" Please add get :vladan to review (or even rubberstamp via IRC) this change only. I can't imagine it being an issue, but...
Attachment #8693628 - Flags: review?(dmose) → feedback+
> + <meta property="og:description" content="Connect easily over video with anyone, anywhere. No downloads or registration." /> > + <meta property="og:title" content="Join and create video calls free with Firefox Hello" /> Sevaan, Matej, these two lines seem like they belong to the old user journey. Do we have new wording for the new user journey, or should we go for these for now? Note that these strings do not need to ride the trains, so if we have to change them in another bug, we can do that.
Flags: needinfo?(sfranks)
Flags: needinfo?(matej)
Flags: needinfo?(sfranks) → needinfo?(frios)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #57) > > + <meta property="og:description" content="Connect easily over video with anyone, anywhere. No downloads or registration." /> > > + <meta property="og:title" content="Join and create video calls free with Firefox Hello" /> > > Sevaan, Matej, these two lines seem like they belong to the old user > journey. Do we have new wording for the new user journey, or should we go > for these for now? I definitely think we should change these, but I don't remember what strings we landed on elsewhere. Fabio would know best.
Flags: needinfo?(matej)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #56) > ::: browser/extensions/loop/content/panels/js/roomStore.js > @@ +368,5 @@ > > + var from = actionData.from; > > + var bucket = this._constants.SHARING_ROOM_URL["FACEBOOK_FROM_" + from.toUpperCase()]; > > + if (typeof bucket === "undefined") { > > + console.error("No URL sharing type bucket found for '" + from + "'"); > > + return; > > Even if we can't log it in telemetry, don't we still want to notify the UI > tour, instead of returning early? Yeah, just copied the style of previous function without thinking much about the logic (tsk tsk tsk). It makes more sense to Notify after opening URL, and keep Telemetry at the end. > > ::: browser/extensions/loop/content/panels/js/roomViews.jsx > @@ +246,2 @@ > > onEditContextClose: React.PropTypes.func, > > + onAddContextClick: React.PropTypes.func, > > I'm not sure why the order got flipped here, but eslint doesn't like it, > since it's no longer alphabetical. Please change this back. I'm not even aware of making this change oÔ > > ::: browser/extensions/loop/content/shared/css/conversation.css > @@ +236,5 @@ > > left: -10rem; > > margin: .5rem 0 0; > > position: absolute; > > right: -10rem; > > + font-size: 1.1rem; /* 8pt */ > > The point of putting something in rems in general is so that its absolute > size can vary as a function of root element's font size. Meaning that if > that changes size, this comment will no longer be true. I'd propose > removing this comment and the other one like it, unless there's some reason > we'd want to preserve 8pt absolute height if we changed the root em. In > which case, we should just use absolute units here and include a comment > explaining why. I suspect we want the former and not the latter. > > But perhaps I'm missing something? I just added the comment based on :sevaan's comment 52, that uses pt. I'd rather keep the relative size, but thought it wouldn't hurt to add a comment on why the specific size used. Maybe I should just add more info to the comment instead? - e.g. /* equal to 8pt with 10px base size */ - or start lashing UX when they use pt? (way more fun with this solution) ...anyway, all the comments addressed on incoming patch
Lashing UX is fine. 8pt = 0.8rem 12 pt = 1.2rem
Changes added, just waiting for ni on new strings
Attachment #8693628 - Attachment is obsolete: true
Attachment #8694708 - Flags: review?(dmose)
Comment on attachment 8694708 [details] [diff] [review] Facebook share button in conversation when waiting alone Review of attachment 8694708 [details] [diff] [review]: ----------------------------------------------------------------- Vladan, I'm assuming the Histograms.json change is cool, can you have a quick look just in case?
Attachment #8694708 - Flags: review?(vladan.bugzilla)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #62) > Comment on attachment 8694708 [details] [diff] [review] > Facebook share button in conversation when waiting alone > > Review of attachment 8694708 [details] [diff] [review]: > ----------------------------------------------------------------- > > Vladan, I'm assuming the Histograms.json change is cool, can you have a > quick look just in case? oh, I assumed that comment 24 did that for us, as related code didn't change since then
Comment on attachment 8694708 [details] [diff] [review] Facebook share button in conversation when waiting alone Quite right, I didn't see that. Never mind, :vladan!
Attachment #8694708 - Flags: review?(vladan.bugzilla)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #57) > > + <meta property="og:description" content="Connect easily over video with anyone, anywhere. No downloads or registration." /> > > + <meta property="og:title" content="Join and create video calls free with Firefox Hello" /> > > Sevaan, Matej, these two lines seem like they belong to the old user > journey. Do we have new wording for the new user journey, or should we go > for these for now? > > Note that these strings do not need to ride the trains, so if we have to > change them in another bug, we can do that. Here are the strings we agreed on bug 1213906. Can you confirm if we can insert the website domain within the "title" as shown below?: - Message Entered by the user. Facebook won't let us force this - The title: "A friend would like to browse [populate website domain here] with you" - The shared URL: <Conversation URL> - The description: "Plan together. Work together. Laugh together."
Flags: needinfo?(frios)
If we can't insert the shared URL within the "title" as shown in comment 65, we can go with this: Title: A friend has invited you to browse the Web together on Firefox Hello Description: Click to connect now. Matej, looks good to you?
Flags: needinfo?(matej)
"Click to connect now" doesn't feel quite right, because it takes you to a tab where you have to click on a "Join" button to connect.
Comment on attachment 8694708 [details] [diff] [review] Facebook share button in conversation when waiting alone Review of attachment 8694708 [details] [diff] [review]: ----------------------------------------------------------------- r=dmose, with tweaks discussed yesterday face-to-face.
Attachment #8694708 - Flags: review?(dmose) → review+
(In reply to Fabio Rios [:frios] from comment #66) > If we can't insert the shared URL within the "title" as shown in comment 65, > we can go with this: > > Title: A friend has invited you to browse the Web together on Firefox Hello > Description: Click to connect now. > > Matej, looks good to you? I would say "with them" instead of "together," but otherwise looks good. Thanks.
Flags: needinfo?(matej)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #67) > "Click to connect now" doesn't feel quite right, because it takes you to a > tab where you have to click on a "Join" button to connect. Dan, we can rephrase. Any thoughts? Maybe... "Join them now" Just need to explain what happens after you click on the link/image. It's not a one-click connect, but they need to know what the result will be.
To me, "now" implies that clicking the button will start the call, which it doesn't. It takes you to the next screen, which has a "Join the Conversation" button. How about just "Click to connect" or "Click to Connect"?
Flags: needinfo?(frios)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #71) > To me, "now" implies that clicking the button will start the call, which it > doesn't. It takes you to the next screen, which has a "Join the > Conversation" button. How about just "Click to connect" or "Click to > Connect"? "Click to connect" works for me. Are we able to make changes to these phrases later? Let's say February? Much of the marketing material will be assembled in lat January, if we identify a better phrase for the entire invite. Can we have it replaced? Also, the "Join the Conversation" button needs to change. Do we have a bug for that?
Flags: needinfo?(frios) → needinfo?(dmose)
(In reply to Fabio Rios [:frios] from comment #72) > (In reply to Dan Mosedale (:dmose) - use needinfo flag for response from > comment #71) > > To me, "now" implies that clicking the button will start the call, which it > > doesn't. It takes you to the next screen, which has a "Join the > > Conversation" button. How about just "Click to connect" or "Click to > > Connect"? > > "Click to connect" works for me. Great! > Are we able to make changes to these > phrases later? Let's say February? Much of the marketing material will be > assembled in lat January, if we identify a better phrase for the entire > invite. Can we have it replaced? The intent as I understand it is that we should be able to replace strings and get them into the field in a small number of weeks. As far as February goes, needinfo-ing Standard8 in case there's something I don't know about that would cause I problem there. > Also, the "Join the Conversation" button needs to change. Do we have a bug > for that? I bet Mark would know this too...
Flags: needinfo?(dmose) → needinfo?(standard8)
(In reply to Dan Mosedale (:dmose) - use needinfo flag for response from comment #73) > (In reply to Fabio Rios [:frios] from comment #72) > > Are we able to make changes to these > > phrases later? Let's say February? Much of the marketing material will be > > assembled in lat January, if we identify a better phrase for the entire > > invite. Can we have it replaced? > > The intent as I understand it is that we should be able to replace strings > and get them into the field in a small number of weeks. As far as February > goes, needinfo-ing Standard8 in case there's something I don't know about > that would cause I problem there. We're going to need a lead time of about 4 weeks for changes to strings. Some of this is to allow localisers time to translate as well as for us getting them into the source. > > Also, the "Join the Conversation" button needs to change. Do we have a bug > > for that? > > I bet Mark would know this too... Depends what the change is that's required. There's some UX for the new web sharing that we haven't implemented yet. RT would know more in that instance.
Flags: needinfo?(standard8) → needinfo?(frios)
Flags: needinfo?(frios)
Rank: 19 → 10
Updating with latests nits and string changes. Ready to merge Not sure if needs r+ flag after :dmose approved the previous one, but adding flag just in case
Attachment #8694708 - Attachment is obsolete: true
Attachment #8703650 - Flags: review?(dmose)
Keywords: checkin-needed
(In reply to Fernando Campo (:fcampo) from comment #75) > Created attachment 8703650 [details] [diff] [review] > Facebook share button in conversation when waiting alone > > Updating with latests nits and string changes. Ready to merge > Not sure if needs r+ flag after :dmose approved the previous one, but adding > flag just in case removing c-n till we know if we need review here
Keywords: checkin-needed
No need for me to re-review after just nits and string changes.
Comment on attachment 8703650 [details] [diff] [review] Facebook share button in conversation when waiting alone Carrying forward previous r+.
Attachment #8703650 - Flags: review?(dmose) → review+
Fernando: I just tried testing this before landing it and came across the fact that it wouldn't load the conversation window correctly: No string found for key: invite_facebook_button2 Can you fix the patch and re-verify it please?
Flags: needinfo?(fernando.campo)
argh, so sorry, changing right now
Flags: needinfo?(fernando.campo)
Fixed and working Carrying over the r+
Attachment #8703650 - Attachment is obsolete: true
Attachment #8704311 - Flags: review+
Attached image Final look (deleted) —
This is how it looks on the final patch
Thanks Fernando, the snapshot shows "On your own timeline" as default whereas we discussed that Facebook Send can allow the default option to be "In a private message" (see Comment 30)? Please confirm if the default behavior is "In a private message"?
I've now landed this as its been a long time and we need to get the last things that are landing directly into m-c landed (I'll merge it to the github repo once it lands in m-c). Please can we get follow-up bugs filed for l10n, and if the private message bit needs a change, address that in a new bug as well.
Iteration: 45.3 - Dec 14 → 46.2 - Jan 11
(In reply to Romain Testard [:RT] from comment #83) > Thanks Fernando, the snapshot shows "On your own timeline" as default > whereas we discussed that Facebook Send can allow the default option to be > "In a private message" (see Comment 30)? Please confirm if the default > behavior is "In a private message"? We can't default 'in a private message' with the facebook sharer, as it doesn't admit any parameters. For that, we need to use the Facebook Send as you said, and it requires the app_id. I recommend following Mark suggestion, and leave it for follow-up after we decide on the app_id
https://hg.mozilla.org/mozilla-central/rev/444097473d1a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Flags: qe-verify+
QA Contact: bogdan.maris
Depends on: 1240045
We did some exploratory testing around the Facebook icon using latest Nightly 46.0a1 across platforms (Windows 10 64 Dell XPS 12, Ubuntu 14.04 32-bit, Mac OS X 10.11 and Windows 7 64-bit) and we only ran into one issue, bug 1240045, that may or may not be related to bug 1228728 (here we don't toggle with pixel ratio to ran into this).
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: