Closed Bug 1000127 Opened 11 years ago Closed 10 years ago

Standalone UI for link clickers needs call initiation

Categories

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

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
34 Sprint 1- 8/4

People

(Reporter: RT, Assigned: aoprea)

References

Details

(Whiteboard: [p=2])

User Story

As a WebRTC browser but not on Firefox URL clicker, my browser opens the URL on a new Tab and I can decide to call-back the URL generator with audio only or video, so that I can define if the callee will see me or not.

Attachments

(1 file, 5 obsolete files)

Implement new standalone UI link clicker
126.24 KB, patch
dmosedale
: review+
Details | Diff | Splinter Review
No description provided.
User Story: (updated)
User Story: (updated)
Summary: Link clicker UI needs ability to initiate the call with audio only or audio+video → Standalone UI for link clickers needs call initiation UI
Depends on: 1000228
Depends on: 1000237
Depends on: 1000240
Depends on: 1000259
User Story: (updated)
No longer depends on: 1000228
No longer depends on: 1000237
No longer depends on: 1000240
No longer depends on: 1000259
Priority: -- → P1
Whiteboard: [p=?, s=UI32]
Target Milestone: --- → mozilla32
Whiteboard: [p=?, s=UI32] → [p=?]
Target Milestone: mozilla32 → mozilla33
Summary: Standalone UI for link clickers needs call initiation UI → Standalone UI for link clickers needs call initiation
On the call initiation page the following data should appear: * Friendly name of the other party or e-mail address if friendly name not available * Expiration date of the URL
As a link clicker would the user have enough information to access the Google Avatar of an account-based link generator? The current UX here assumes that we can although I assume we actually can't ? https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start-known By default Google avatars are publicly available (this can be restricted by the user though) although I am not sure that we are able to re-build the URL pointing to the avatar picture from the data available on the link clicker side?
https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start - known stuff on hold (until FxA). Darrin is updating UX with choice of audio and video or audio only answer.
Whiteboard: [p=?] → [p=2]
Assignee: nobody → aoprea
Target Milestone: mozilla33 → 34 Sprint 1- 8/4
Please note that as per discussion with Arcadio yesterday we should not be using the "Firefox Hello" brand for now (likely we'll use it when we get to beta). For Nightly and Aurora we should replace "Hello" by "WebRTC" wherever "Hello" appears in the UX and also use the Firefox logo instead of the speech bubble for now.
Attachment #8464115 - Flags: review?(dmose)
Comment on attachment 8464115 [details] [diff] [review] Implement new standalone UI link clicker Review of attachment 8464115 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but we want to pull the conversation create date info into the model. ::: browser/components/loop/content/shared/css/common.css @@ +97,5 @@ > > .btn-large { > + padding: .5em; > + font-size: 18px; > + max-width: 200px; Please comment on origin of these numbers; and also try using two flex boxes instead of max-width. ::: browser/components/loop/standalone/content/css/webapp.css @@ +47,5 @@ > +.footer a, > +.terms-service, > +.terms-service a { > + font-size: .6rem; > + font-size: 400; I bet this wants to be font-weight. ::: browser/components/loop/standalone/content/index.html @@ +25,5 @@ > + <a href="#">Mozilla 2014</a> > + <a href="#">Privacy Policy</a> > + <a href="#">Get Help</a> > + </p> > + </div> I think this wants to be a React component. I'm not sure whether it needs to be added to readme.html; I suspect so. @@ +47,5 @@ > > <script> > // Set the 'lang' and 'dir' attributes to <html> when the page is translated > window.addEventListener('localized', function() { > + loop.webapp.init(); One nice feature of moving this to webapp.jsx would be that we could then have a test of it. ::: browser/components/loop/standalone/content/js/webapp.jsx @@ +91,5 @@ > + "January", "February", "March", > + "April", "May", "June", > + "July", "August", "September", > + "October", "November", "December" > + ] This needs some l10n love. ::: browser/components/loop/standalone/content/l10n/data.ini @@ +27,5 @@ > +initiate_call_button_label=Click Call to start a video chat > +initiate_call_button=Call > +## LOCALIZATION NOTE (legal_text_and_links): In this item, don't translate the > +## part between {{..}} > +legal_text_and_links=By using this product you agree to the <a target="_blank" href="{{terms_of_use_url}}">Terms of Use</a> and <a href="{{privacy_notice_url}}">Privacy Notice</a>. I think we want something more along the lines of the things described here: https://developer.mozilla.org/de/docs/Mozilla/Localization/Localization_best_practices#Avoid_concatenations.2C_use_placeholders_instead With all of the markup being JSX literals in the .jsx file. ::: browser/components/loop/test/standalone/webapp_test.js @@ +341,5 @@ > }, "{client: <properly constructed client>, outgoing: true}")); > }); > > + it("should make a $.get request to get token timestamp", function(){ > + sinon.assert.calledOnce(jQuery.get); This wants dependency injection of a fake location object, I think, and possibly also a switch to the fakeXMLHttpRequest stuff to make it easy to test the method, path, etc.
Attachment #8464115 - Flags: review?(dmose) → review-
Comment on attachment 8464115 [details] [diff] [review] Implement new standalone UI link clicker Review of attachment 8464115 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for jumping in, but I felt like I needed to comment on some bits here. ::: browser/components/loop/standalone/content/js/webapp.jsx @@ +97,5 @@ > + }, > + > + componentDidMount: function() { > + var conversationToken = location.href.split("/").pop(); > + $.get(loop.config.serverUrl + "/calls/" + conversationToken) Really this wants to be moved to the standaloneClient API. @@ +111,5 @@ > + var month = date.getMonth(); > + var day = date.getDay(); > + var year = date.getFullYear(); > + var timestamp = this.state.months[month].substring(0,3) + > + " " + day + ", " + year; This is unlikely to scale at localization time. See http://static.guim.co.uk/sys-images/Guardian/Pix/pictures/2013/12/15/1387125445310/ef1bbe42-0bdb-414c-af40-6d5d1856b379-460x313.png Also, I'd rather see this being extracted from the component as a shared reusable helper. @@ +124,5 @@ > + /* jshint ignore:start */ > + <header className="container-box"> > + <h1 className="light-weight-font"><strong>Firefox</strong> WebRTC!</h1> > + <img className="loop-logo" alt="Loop logo" > + src="../../content/shared/img/firefox-logo.png" /> I'd rather see this as a styled div, see what's available for expired call url notifiction which showcase the Firefox logo as well. @@ +164,5 @@ > + this.props.notifier.errorL10n("unable_retrieve_call_info"); > + }, > + > + /** > + * Used for adding different styles to the panel This should be updated to reflect that it's also used for conversation as well. Actually, I think this wants to be shared code as it's a pure dupe of what currently sits in conversation.jsx. Tbh, we could just go with adding these classes to the document body, so styling for a given target platform would be just matter of: .foo {} .linux .foo {} @@ +190,5 @@ > * @see https://bugzilla.mozilla.org/show_bug.cgi?id=991126 > */ > + _disableForm: function() { > + var button = this.refs.submitButton.getDOMNode(); > + button.disabled = true; Please don't touch the DOM by hand, rather update comp state so VDom can safely perform its batch DOM updates. @@ +196,5 @@ > > /** > * Initiates the call. > * > * @param {SubmitEvent} event This is no more a SubmitEvent. @@ +226,5 @@ > + <p className="large-font light-weight-font"> > + {__("initiate_call_button_label")} > + </p> > + <div id="messages"></div> > + <div className="button-group"> We'll probably want a reusable component for button groups at some point. Also, it's a bit strange to see this used while we only have a single button here. ::: browser/components/loop/test/standalone/webapp_test.js @@ +341,5 @@ > }, "{client: <properly constructed client>, outgoing: true}")); > }); > > + it("should make a $.get request to get token timestamp", function(){ > + sinon.assert.calledOnce(jQuery.get); Agreed with :dmose for DI, and we probably want to use the standaloneClient anyway. @@ +345,5 @@ > + sinon.assert.calledOnce(jQuery.get); > + }); > + > + it("should set the correct timestamp for the call url", function(done) { > + // XXX is setTimeout the best option? Probably not :) Here you probably want to use state to notify the component it should render according to that new state. @@ +349,5 @@ > + // XXX is setTimeout the best option? > + setTimeout(function() { > + var callUrlDate = view.getDOMNode().querySelector(".call-url-date"); > + > + expect(callUrlDate.innerHTML).to.eql("(From Dec 3, 1969)"); See previous comments about l10n, also I'd expect to see that date value set somewhere in the test.
Attachment #8464115 - Flags: feedback-
(In reply to Nicolas Perriault (:NiKo`) from comment #7) > @@ +111,5 @@ > > + var month = date.getMonth(); > > + var day = date.getDay(); > > + var year = date.getFullYear(); > > + var timestamp = this.state.months[month].substring(0,3) + > > + " " + day + ", " + year; You probably want to use date.toLocaleDateString(); - that should do everything for you, and get the right format.
Attachment #8464115 - Attachment is obsolete: true
Attachment #8464869 - Attachment is obsolete: true
Comment on attachment 8464891 [details] [diff] [review] Implement new standalone UI link clicker Review of attachment 8464891 [details] [diff] [review]: ----------------------------------------------------------------- I was in the area looking at this, as I was trying to work out how getting the extra data didn't trigger a conversation window at the other end and realised its a get rather than a post. With that sorted, I think we can do better on the function for handling the get request: ::: browser/components/loop/content/shared/js/models.js @@ +152,5 @@ > + * > + * @param {string} options.serverUrl Base url where the req is made > + * @param {string} options.location Window.location object > + **/ > + requestCallUrlInfo: function(options) { Please move this into standaloneClient.js. You'll find that the server url is already set up there. Additionally, note that this ConversationModel already has "loopToken" stored as an attribute, so you don't need to do manual extraction from location.href.
Also: note that clientShortname=WebRTC! shouldn't be localisable (as per other bugs).
Blocks: 1019454
(In reply to Mark Banner (:standard8) from comment #13) > Also: note that clientShortname=WebRTC! shouldn't be localisable (as per > other bugs). The idea was that it could be used in more places and easily be changed to "Hello!". Should I add a localization note or not use l10n attributes for this ?
Attachment #8464891 - Attachment is obsolete: true
Attachment #8465912 - Flags: review?(dmose)
Attachment #8465087 - Attachment description: Implement new standalone UI link clicker, v.next → Broken patch upload
Attachment #8465087 - Attachment is obsolete: true
Comment on attachment 8465912 [details] [diff] [review] Implement new standalone UI link clicker Review of attachment 8465912 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; just needs some comments addressed. ::: browser/components/loop/content/shared/js/models.js @@ +173,5 @@ > + console.error("Request failed: ", status); > + console.log(reqObj.responseText); > + }); > + }, > + This whole addition appears to be a leftover and can disappear. ::: browser/components/loop/standalone/content/js/webapp.jsx @@ +87,5 @@ > + var cx = React.addons.classSet; > + var conversationUrl = location.href; > + var urlCreationDateClasses = cx({ > + "light-color-font": true, > + "call-url-date": true, Add a comment that this is for use by the tests. @@ +101,5 @@ > + <h3 className="call-url"> > + {conversationUrl} > + </h3> > + <h4 className={urlCreationDateClasses}> > + (From {this.props.urlCreationDate}) Please make this localizable and downcase the "From". @@ +117,5 @@ > + <div title="Mozilla Logo" className="footer-logo"></div> > + <p className="footer-external-links"> > + <a href="#">Mozilla 2014</a> > + <a href="#">Privacy Policy</a> > + <a href="#">Get Help</a> Let's get rid of the external links and spin them off to another bug. We already havea privacy notice about this, and there's authoritative info on where any of them should go. @@ +141,4 @@ > > + getInitialState: function() { > + return { > + urlCreationDate: false, Let's name this urlCreationDateString, initialize it to '', and set the cx boolean based on string.length being non-zero. @@ +185,3 @@ > }); > + // Disables this form to prevent multiple submissions. > + // @see https://bugzilla.mozilla.org/show_bug.cgi?id=991126 Can delete this bugzilla reference; it doesn't add to code readability. ::: browser/components/loop/test/standalone/standalone_client_test.js @@ +52,5 @@ > + errno: 101, > + error: "error", > + message: "invalid token", > + info: "error info" > + }; Since this is unused, let's discard it. @@ +59,5 @@ > + }); > + > + afterEach(function() { > + sandbox.restore(); > + }); We already do sandbox.restore() at a higher level afterEach, so no need here. @@ +73,5 @@ > + client.requestCallUrlInfo("fakeCallUrlToken", function() {}); > + > + sinon.assert.calledOnce($.get); > + sinon.assert.calledWithExactly($.get, "http://fake.api/calls/fakeCallUrlToken"); > + }); Please use fakeXHR here so that we're testing interface rather than implementation. @@ +84,5 @@ > + }; > + > + client.requestCallUrlInfo("fakeCallUrlToken", successCallback); > + requests[0].respond(200, {"Content-Type": "application/json"}, > + JSON.stringify(serverResponse)); Please add a blank line between the execute and verify sections of the test for readability. @@ +92,5 @@ > + }); > + > + it("should log the error if the requests fails", function() { > + sinon.stub(console, "error"); > + sinon.spy(client, "_failureHandler"); This is used to test an implementation detail, but you're already testing the interface, so this its reference can just be deleted. @@ +93,5 @@ > + > + it("should log the error if the requests fails", function() { > + sinon.stub(console, "error"); > + sinon.spy(client, "_failureHandler"); > + var successCallback = sandbox.spy(function() {}); just use a stub here; no need for a wrapped anon function. @@ +94,5 @@ > + it("should log the error if the requests fails", function() { > + sinon.stub(console, "error"); > + sinon.spy(client, "_failureHandler"); > + var successCallback = sandbox.spy(function() {}); > + var error = JSON.stringify({error:true}); Please add a line between the setup and execute sections for readability. ::: browser/components/loop/test/standalone/webapp_test.js @@ +313,5 @@ > + requestCallUrlInfo: function(token, cb) { > + cb(null, {urlCreationDate: 0}); > + }, > + settings: {baseServerUrl: loop.webapp.baseServerUrl} > + }); I think just using a simple object rather than stub with "returns" would be more readable/maintainable. @@ +389,5 @@ > + }); > + > + it("should listen for session:error events", function() { > + sinon.assert.calledWithExactly(conversation.listenTo, conversation, > + "session:error", sinon.match.func); .calledOnce here should work, I think. ::: browser/locales/en-US/chrome/browser/loop/loop.properties @@ +30,5 @@ > call_has_ended=Your call has ended. > close_window=Close this window > > +initiate_call_button_label=Click Call to start a video chat > +initiate_call_button=Call AFAIK, we only need these strings for the link-clicker (i.e. currently in data.ini), so these can be removed.
Attachment #8465912 - Flags: review?(dmose) → review-
Attachment #8465912 - Attachment is obsolete: true
Comment on attachment 8466523 [details] [diff] [review] Implement new standalone UI link clicker Review of attachment 8466523 [details] [diff] [review]: ----------------------------------------------------------------- Looks good; r=dmose
Attachment #8466523 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2898485e4c8e
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I'm not seeing UI that is representative of the mockup here: https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start How do I test that this is working as expected?
(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #22) > I'm not seeing UI that is representative of the mockup here: > https://people.mozilla.org/~dhenein/labs/loop-link-spec/#call-start > > How do I test that this is working as expected? :ashughes This is part of the standalone client and hasn't been deployed yet - its being deployed in bug 1053389.
(In reply to Mark Banner (:standard8) from comment #23) > This is part of the standalone client and hasn't been deployed yet - its being deployed in bug 1053389. Thanks Mark. I'll wait for that to be resolved before testing. As an aside, will this have sufficient in-testsuite coverage or will it require manual smoketesting?
Depends on: 1053389
Flags: qe-verify+
QA Contact: anthony.s.hughes
This should be covered be the in-tree tests I think.
(In reply to Mark Banner (:standard8) from comment #25) > This should be covered be the in-tree tests I think. Thanks, untracking for QE verification.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: