1199815 - Replace Error Summary at bottom of ui-showcase with React Component

Status: RESOLVED FIXED

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

Comment 1

[Fernando Campo (:fcampo)]

Attachment: Replace Error Summary at bottom of ui-showcase with React Component

Comment 2

[Fernando Campo (:fcampo)]

Comment on attachment 8677287 [details] [diff] [review]
Replace Error Summary at bottom of ui-showcase with React Component

also added missing <ul> tag

Comment 3

[Mark Banner (:standard8)]

Comment on attachment 8677287 [details] [diff] [review]
Replace Error Summary at bottom of ui-showcase with React Component

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

This is looking good. There's some eslint failures that need addressing - if you run eslint or use the run-all-loop-tests.sh, it should pick them up. You might also be able to add eslint into your editor as well.

The rest of the comments are getting you closer to our normal styles, and a couple of bits of tidy up I think we could do whilst we're in the area.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +1312,5 @@
> +      summary: React.PropTypes.string.isRequired,
> +      errorLine1: React.PropTypes.string.isRequired,
> +      errorLine2: React.PropTypes.string.isRequired
> +    },
> +    render: function() {

general nit: Please add a blank line in between object functions/properties.

@@ +1314,5 @@
> +      errorLine2: React.PropTypes.string.isRequired
> +    },
> +    render: function() {
> +      return (
> +        <li className = "test fail">

general nit: Generally in jsx code we do `className="test fail"`, i.e. no spaces either side of equals.

@@ +1318,5 @@
> +        <li className = "test fail">
> +          <h2>
> +            {this.props.summary}
> +          </h2>
> +          <pre class="error">

This should be className.

@@ +1335,5 @@
> +    },
> +    render: function() {
> +      var expectedWarnings = 0;
> +      var mismatch = this.props.warnings &&
> +                     this.props.warnings.length !== expectedWarnings;

Not your fault, but now we're down at zero, I think we can simplify this to something like `!!this.props.warnings.length`

@@ +1342,5 @@
> +      var warningMsg = null;
> +      if (mismatch) {
> +        warningMsg = <Failure summary = 'Unexpected number of warnings detected in UI-Showcase'
> +                              errorLine1 = {'Got: ' + this.props.warnings.length}
> +                              errorLine2 = {'Expected: ' + expectedWarnings} />

Whilst you're here, could you change this to "Unexpected warnings detected rendering UI-Showcase", and then just print out the Got?

@@ +1359,5 @@
> +            <em>{totalFailures}</em>
> +          </div>
> +          <ul>
> +            {warningMsg}
> +            {errorMsg}

It might be nicer to declare the <Failure .../> inline here, and pass in the `mismatch` or `this.props.error` as something like a `showErrors` property.

The Failure class would then return null if showErrors is false.

This would have the advantage of simplifying the readability, by not having to work through the various if statements.

Comment 4

[Fernando Campo (:fcampo)]

Attachment: Replace Error Summary at bottom of ui-showcase with React Component

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 8678798 [details] [diff] [review]
Replace Error Summary at bottom of ui-showcase with React Component

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

This is looking much better, but I now get:

TypeError: this.props.error is undefined

when I try and run it.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +1341,5 @@
> +    },
> +
> +    render: function() {
> +      // if no errors, return blank
> +      return this.props.errorDetected ? (

I slightly prefer this format:

if (!this.props.errorDetected) {
  return null;
}
...

As this reduces complexity with visually reading the code (you get the exception case out the way without having to find the other end at the end of the function).

Comment 6

[Fernando Campo (:fcampo)]

Attachment: Replace Error Summary at bottom of ui-showcase with React Component

all nits done, totally missed the console error the first time (tested only with existing error and warnings)

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8679513 [details] [diff] [review]
Replace Error Summary at bottom of ui-showcase with React Component

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

Looks good, r=Standard8

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e90655d486ca

Comment 9

[Carsten Book [:Tomcat]]

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