Closed
Bug 1199815
Opened 9 years ago
Closed 9 years ago
Replace Error Summary at bottom of ui-showcase with React Component
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox44 fixed)
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: dcritchley, Assigned: fcampo)
References
Details
(Whiteboard: [tech-debt][lang=js])
Attachments
(1 file, 2 obsolete files)
10.63 KB,
patch
|
standard8
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Updated•9 years ago
|
Updated•9 years ago
|
Rank: 27
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → fernando.campo
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Comment on attachment 8677287 [details] [diff] [review] Replace Error Summary at bottom of ui-showcase with React Component also added missing <ul> tag
Attachment #8677287 -
Flags: review?(standard8)
Comment 3•9 years ago
|
||
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.
Attachment #8677287 -
Flags: review?(standard8) → review-
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8677287 -
Attachment is obsolete: true
Attachment #8678798 -
Flags: review?(standard8)
Comment 5•9 years ago
|
||
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).
Attachment #8678798 -
Flags: review?(standard8)
Assignee | ||
Comment 6•9 years ago
|
||
all nits done, totally missed the console error the first time (tested only with existing error and warnings)
Attachment #8678798 -
Attachment is obsolete: true
Attachment #8679513 -
Flags: review?(standard8)
Comment 7•9 years ago
|
||
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
Attachment #8679513 -
Flags: review?(standard8) → review+
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e90655d486ca
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•9 years ago
|
Iteration: --- → 44.3 - Nov 2
You need to log in
before you can comment on or make changes to this bug.
Description
•