Closed Bug 1276206 Opened 9 years ago Closed 8 years ago

Reps: If a grip/object's property name is "more", it is clobbers the caption

Categories

(DevTools :: Shared Components, defect, P1)

Product:
DevTools
Component:
Shared Components
Type:
defect

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Iteration:
52.2 - Oct 17
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: linclark, Assigned: Honza)

References

Details

(Whiteboard: [reserve-html])

Attachments

(1 file, 2 obsolete files)

bug1276206.patch
(deleted), patch
linclark
: review+
Details | Diff | Splinter Review
Because Reps use the key "more" for captions, they will collide with any properties named more. For example, if you have the following object: > `{a: "a", b: "b", more: "c", d: "d", e: "e"}` The output would be: > `{a: "a", b: "b", more: "c"}` But it should be: > `{a: "a", b: "b", more: "c", more...}`
Priority: -- → P1
Summary: If a grip/object's property name is "more", it is clobbers the caption → Reps: If a grip/object's property name is "more", it is clobbers the caption
Whiteboard: [devtools-html] [triage]
Attached patch Bug1276206.patch (obsolete) (deleted) — — Splinter Review
Removing the key is fine in this case. React will use its default, which is the array index of the Caption element. The "more..." caption should never be at a different position in the array, so using index as the key should have the same effect.
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attachment #8757279 - Flags: review?(odvarko)
Comment on attachment 8757279 [details] [diff] [review] Bug1276206.patch Review of attachment 8757279 [details] [diff] [review]: ----------------------------------------------------------------- Good catch! Honza
Attachment #8757279 - Flags: review?(odvarko) → review+
Hm... I am now seeing: "Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `Grip`. See https://fb.me/react-warning-keys for more information." Honza
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify-
Whiteboard: [devtools-html] [triage] → [devtools-html]
Attached patch Bug1276206.patch (obsolete) (deleted) — — Splinter Review
I realized that if we move the caption out of the array and just pass it in directly in the children arguments, as a sibling to props, then we won't get the warning. Honza, what would you think of an approach like this? Not fully implemented yet, just wanted to get your feedback first.
Attachment #8757279 - Attachment is obsolete: true
Attachment #8757319 - Flags: feedback?(odvarko)
(In reply to Lin Clark [:linclark] from comment #4) > Created attachment 8757319 [details] [diff] [review] > Bug1276206.patch > > I realized that if we move the caption out of the array and just pass it in > directly in the children arguments, as a sibling to props, then we won't get > the warning. > > Honza, what would you think of an approach like this? Not fully implemented > yet, just wanted to get your feedback first. Interesting, I didn't know keys work that way. Anyway, works for me. Honza
Attachment #8757319 - Flags: feedback?(odvarko) → feedback+
Iteration: 49.3 - Jun 6 → 50.1
Depends on: 1264685
Blocks: 1276210
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.1 → ---
Priority: P1 → P2
I would like to work on this. Can Someone help me where to start?
I have a patch up for this that I just have to apply a few changes to. Sorry that wasn't clearer, I didn't realize that my assignment to myself had been removed. Another issue that would probably be good is this one: https://bugzilla.mozilla.org/show_bug.cgi?id=1264686
Assignee: nobody → lclark
(In reply to Lin Clark [:linclark] from comment #7) > I have a patch up for this that I just have to apply a few changes to. Sorry > that wasn't clearer, I didn't realize that my assignment to myself had been > removed. > > Another issue that would probably be good is this one: > https://bugzilla.mozilla.org/show_bug.cgi?id=1264686 That depends on some other bug can you give me a good bug similar but free to take?
No longer blocks: 1257552
Status: NEW → ASSIGNED
Iteration: --- → 51.1 - Aug 15
Priority: P2 → P1
Iteration: 51.1 - Aug 15 → 51.2 - Aug 29
Iteration: 51.2 - Aug 29 → ---
Priority: P1 → P3
Whiteboard: [devtools-html] → [reserve-html]
Status: ASSIGNED → NEW
Assignee: lclark → odvarko
Status: NEW → ASSIGNED
Iteration: --- → 52.1 - Oct 3
Priority: P3 → P1
Honza, if you're working on this be advised there's an easier way to handle this. What you can do is use the rest syntax, e.g. ...values, to turn from an array to individual children. Then you don't need keys. As long as we aren't reordering any children, I think this is the best solution
Attached patch bug1276206.patch (deleted) — — Splinter Review
(In reply to Lin Clark [:linclark] from comment #9) > Honza, if you're working on this be advised there's an easier way to handle > this. > > What you can do is use the rest syntax, e.g. ...values, to turn from an > array to individual children. Then you don't need keys. As long as we aren't > reordering any children, I think this is the best solution Nice, I've done it that way. - Object rep fixed + test updated. - Grip rep fixed + test updated. - Array/GripArray also changed, but just to make the code a bit more consistent. These reps don't have the problem (that's way there are no tests for these two). Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b96ea37cd4b Honza
Attachment #8757319 - Attachment is obsolete: true
Attachment #8795312 - Flags: review?(lclark)
Iteration: 52.1 - Oct 3 → 52.2 - Oct 17
Comment on attachment 8795312 [details] [diff] [review] bug1276206.patch Review of attachment 8795312 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, thanks!
Attachment #8795312 - Flags: review?(lclark) → review+
Thanks Lin! Honza
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/fx-team/rev/327bd3387ff6 Avoid caption clobber if more prop exists. r=linclark
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/327bd3387ff6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: