1042799 - The readme.html file should showcase actual React components

Status: RESOLVED FIXED

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

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

As discussed today during our daily standup, the Loop readme.html file should be updated to directly use the React components used by the app, to avoid HTML code duplication and help spotting cross visual regressions more acurately.

Comment 1

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

So it seems harder than expected and raises the following issues:

- if we want to showcase all components, loop/content/shared/css/readme.html is probably not the right place because we can’t easily access components (and related styles) for desktop and standalone

- there’s a big issue with l10n; desktop and standalone are using two different libs and source files for strings, which makes components using the right ones nearly impossible without using *very* ugly hacks (and we really want to render the components using our real strings)

- it’s also not obvious how we could load both source files for desktop & standalone l10n strings (namely loop.properties & data.ini); {moz|web}L10n seems to be only capable of dealing with a single source file…

- rendering a component like ConversationView implies having its ConversationModel instance dependency properly configured, and as it’s tightly coupled to an SDK session, you’d need to initiate a real video communication using the SDK to render it properly (this is admittedly overkill); mocking out the SDK to simulate a fake ongoing video communication is probably out of question as well. If the SDK provided access to raw media streams, we could decouple the component enough from it so the situation would be infinitely better… 

Now, I don’t know what to do with this; the l10n and sdk dependency issues seems to be major blockers, and turn me quite pessimistic with this approach right now… Thoughts?

Also note that current state of investigation is viewable as code here: https://github.com/n1k0/gecko-dev/tree/bug-1042799-readme-html-showcases-react-components/browser/components/loop/ui (ugly code is ugly, just WiP there)

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: WiP-preview.png

WiP preview.

Comment 3

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: WiP-preview.png

WiP preview

Comment 4

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1042799-Loop-UI-components-showcase-WiP.patch

This WiP patch sits on top of patch for bug 1000131 (Part 2) and brings a page showcasing most of the React components available in Loop.

It uses an ugly hack for faking both mozL10n and webL10n and makes then returning a fake string everytime a translation is requested; but the patch serves the purpose of checking for styling regression, so I think we could live with that until we fix this issue as it's imho a valuable tool for developers.

Next step would be to completely remove the readme.html file.

Comment 5

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: loop-ui-showcase-demo.gif

Quick demo as a(n heavy) GIF.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Since this is labelled WiP, I assume you're actually interested in feedback? rather than review? and then landing.

In any case, this looks like a great start!


One thought I had about the UX is that it would be nice to have a mode where everything is laid out with minimal padding, with things packed as tightly as possible (should be mostly doable with inline or inline-block, I'd think).  The idea would be that devs could expand it to fill their monitors, and if they're lucky, get all or most of the views onscreen at once.  This would be handy for changing common.css, hitting reload, and seeing all possible side-effects as apparent visual motion across the reload.

I don't actually feel like the l10n thing is a horrid hack at all.  It's a great response to the fact the our l10n setup is currently weird.  I'd propose not trying to do anything better until we see what l10n changes shake out of the deployment strategy bits we're hoping to dig into on Monday.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8461571 [details] [diff] [review]
0001-Bug-1042799-Loop-UI-components-showcase-WiP.patch

Review of attachment 8461571 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/panel.jsx
@@ +8,5 @@
>  /*global loop:true, React */
>  
> +navigator.mozLoop = {
> +  getLoopCharPref: function() {}
> +};

Why do you need this? Smells like a hack to me...

::: browser/components/loop/ui/ui-showcase.css
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +.showcase {

I didn't see you use this className in ui-showcase.jsx... so how does this work?

::: browser/components/loop/ui/ui-showcase.jsx
@@ +20,5 @@
> +  var CallUrlExpiredView = loop.webapp.CallUrlExpiredView;
> +
> +  // Shared components
> +  var ConversationToolbar = loop.shared.views.ConversationToolbar,
> +      ConversationView = loop.shared.views.ConversationView;

Please use a `var` declaration for each variable.

Comment 8

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1042799-Loop-UI-components-showcase-rev2.patch

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> Why do you need this? Smells like a hack to me...

It's been put in the wrong file; moved to ui-showcase.jsx. It basically fakes the mozLoop object which doen't exist in regular webpages, so things don't blow up :)

> I didn't see you use this className in ui-showcase.jsx... so how does this
> work?

It was attached to the body tag in index.html, but I moved it to the ShowCase component.

> Please use a `var` declaration for each variable.

Fixed.

Comment 9

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1042799-Loop-UI-components-showcase-rev3.patch

Missing file in previsously attached patch, sorry.

Comment 10

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: 0001-Bug-1042799-Loop-UI-components-showcase-rev4.patch

Fully removed now obsolete readme.html file as suggested in a previous comment.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8462531 [details] [diff] [review]
0001-Bug-1042799-Loop-UI-components-showcase-rev4.patch

Review of attachment 8462531 [details] [diff] [review]:
-----------------------------------------------------------------

Alright, looks good Niko!

Can you file a follow-up for the request that Dan made re. showing all UI components in one file?

I did find a couple of things that I think you should look at before you land this (or someone else does that for you).

r=me with comments addressed.

::: browser/components/loop/content/js/conversation.jsx
@@ +24,4 @@
>    var IncomingCallView = React.createClass({
>  
>      propTypes: {
> +      model: React.PropTypes.object.isRequired

I missed this the first time, but it does raise a question: Why is this needed for the ui-showcase to work?

::: browser/components/loop/content/shared/css/conversation.css
@@ +20,4 @@
>    padding: 0;
>  }
>  
> +.conversation-toolbar li {

Basically the same comment as I put in `ui-showcase.css`.

::: browser/components/loop/ui/fake-l10n.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

The license header got updated, please change this to

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

(You can always find the correct headers at https://www.mozilla.org/MPL/headers/)

@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * /!\ FIXME: THIS IS AN HORRID HACK which fakes both the mozL10n and webL10n

nit: A HORRID HACK

::: browser/components/loop/ui/fake-mozLoop.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

update the header here too

::: browser/components/loop/ui/ui-showcase.css
@@ +6,5 @@
> +  width: 730px;
> +  margin: 0 auto;
> +}
> +
> +.showcase header {

I *think* you can use child-selectors almost everywhere in this file.

It's good practice to start thinking more about writing efficient selectors and applying these simple rules in Loop CSS. See this excellent article: https://developer.mozilla.org/en-US/docs/Web/Guide/CSS/Writing_efficient_CSS

Comment 12

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Loop UI components showcase, r=mikedeboer

Rebased patch against latest master.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8463606 [details] [diff] [review]
Loop UI components showcase, r=mikedeboer

Review of attachment 8463606 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! Thanks, Niko!

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed99f2f5561a

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ed99f2f5561a

Comment 16

[u279076]

Untracking for QA. Please needinfo me to request testing.