Closed
Bug 1283522
Opened 8 years ago
Closed 8 years ago
Reps: support -0 grip in number rep
Categories
(DevTools :: Shared Components, defect, P1)
DevTools
Shared Components
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: linclark, Assigned: steveck)
References
Details
(Whiteboard: [devtools-html])
Attachments
(2 files, 2 obsolete files)
Currently the number rep supports -0 as a value but not as a grip. It should support both cases.
Updated•8 years ago
|
Blocks: devtools-html-2
Flags: qe-verify-
Reporter | ||
Comment 1•8 years ago
|
||
Here's a failing test.
Updated•8 years ago
|
Priority: -- → P2
Whiteboard: [devtools-html] [triage] → [devtools-html]
Updated•8 years ago
|
Assignee: nobody → schung
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years ago
|
||
Hi Lin,
Since it's my first patch in firefox/devtool and I'm not 100% sure that it's correct direction of fixing the issue, I just created a quick patch based on your failing test patch. I'll submit the patch by mozreview if you think the patch is ok, thanks!
Attachment #8769582 -
Flags: feedback?(lclark)
Reporter | ||
Comment 3•8 years ago
|
||
Comment on attachment 8769582 [details] [diff] [review]
Bug-1283522-fix.patch
Review of attachment 8769582 [details] [diff] [review]:
-----------------------------------------------------------------
Overall this looks good, thanks!
You'll want to run the changes through our ESLint checker. To do that, run ./mach eslint --no-ignore <filename>
Once you've fixed any issues that that uncovers, you can post another patch and add me for review.
::: devtools/client/shared/components/reps/number.js
@@ +20,5 @@
> displayName: "Number",
>
> stringify: function (object) {
> + let isNegativeZero = Object.is(object, -0) ||
> + (object.type && object.type == "-0"); //grip
Our code style doesn't allow for comments at the end of a line. I don't think that this needs a comment, since we use the grips extensively in this part of the system.
::: devtools/client/shared/components/test/mochitest/test_reps_number.html
@@ +56,5 @@
> + let renderedComponent = renderComponent(Number.rep, { object: getGripStub("testNegZeroGrip") });
> + is(renderedComponent.textContent, "-0", "Number rep has expected text content for negative zero");
> +
> + renderedComponent = renderComponent(Number.rep, { object: getGripStub("testNegZeroValue") });
> + is(renderedComponent.textContent, "-0", "Number rep has expected text content for negative zero");
For both of these, the string "Number rep has expected text content for negative zero" should have something at the end to indicate whether it's testing the value -0 or the grip form. That will make it easier to figure out which test is failing if it fails.
Attachment #8769582 -
Flags: feedback?(lclark) → feedback+
Updated•8 years ago
|
Iteration: --- → 50.3 - Jul 18
Priority: P2 → P1
Assignee | ||
Comment 4•8 years ago
|
||
Thanks for the review! I addressed some issues and resent the patch again before my Mozreview permission is ready.
Attachment #8769582 -
Attachment is obsolete: true
Attachment #8770460 -
Flags: review?(lclark)
Reporter | ||
Updated•8 years ago
|
Attachment #8766808 -
Attachment is obsolete: true
Reporter | ||
Comment 5•8 years ago
|
||
Comment on attachment 8770460 [details] [diff] [review]
Bug-1283522-fix.patch
Review of attachment 8770460 [details] [diff] [review]:
-----------------------------------------------------------------
This looks good to me. Can you push to try? If so, please make sure to push it up to try and paste the link here. If the ES check passes and none of the devtools tests fail, then you can add the "checkin-needed" keyword.
Thanks for your work on this!
Attachment #8770460 -
Flags: review?(lclark) → review+
Assignee | ||
Comment 6•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64134/
Attachment #8770794 -
Flags: review?(lclark)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8770794 [details]
Bug 1283522 - Reps: support -0 grip in number rep.
https://reviewboard.mozilla.org/r/64134/#review61268
The only failure is unrelated, so this is good to go.
Attachment #8770794 -
Flags: review?(lclark) → review+
Pushed by lclark@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e1db3f6c752
Reps: support -0 grip in number rep. r=linclark
Comment 9•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•