Closed Bug 1276210 Opened 8 years ago Closed 8 years ago

Reps: Use getDefaultProps for Caption object

Categories

(DevTools :: Shared Components, defect)

Product:
DevTools
Component:
Shared Components
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 affected)

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox49 --- affected

People

(Reporter: linclark, Unassigned)

References

Details

Attachments

(1 file)

Bug1276210.patch
(deleted), patch
Honza
: review+
Details | Diff | Splinter Review
Currently, reps pass in the object to Caption. However, all the reps use the same caption. We could make this a default prop. >getDefaultProps: function() { > return { > object: "more..." > }; > }
Attached patch Bug1276210.patch (deleted) — — Splinter Review
Assignee: nobody → lclark
Status: NEW → ASSIGNED
Attachment #8757281 - Flags: review?(odvarko)
Comment on attachment 8757281 [details] [diff] [review] Bug1276210.patch Yep, I like this! Tested on Grip and works for me. Honza
Attachment #8757281 - Flags: review?(odvarko) → review+
Whiteboard: [devtools-html] [triage]
Blocks: 1257552, devtools-html-2
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Iteration: 49.3 - Jun 6 → 50.1
Depends on: 1276206
Blocks: 1264678
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.1 → ---
Priority: P1 → P2
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
I saw the patch is r+ and not checked-in-ed. Any related work needed before check in the patch?
Flags: needinfo?(lclark)
Thank you for the offer to help, but I am managing this as part of my own work on reps. I'll mark it as ready when we plan to land it.
Flags: needinfo?(lclark)
Is this assigned? Or can I take it?
Yes, you can take it. There's a patch attached which does most of the work. However, it might not apply. In the patch where it has the following: > items.push(Caption({})); It should be: > items.push(Caption({key: "more"}));
(In reply to Lin Clark [:linclark] from comment #6) > Yes, you can take it. There's a patch attached which does most of the work. > However, it might not apply. In the patch where it has the following: > > > items.push(Caption({})); > > It should be: > > > items.push(Caption({key: "more"})); But this depends on some other bug right? is it ok to continue to work ?
It's ok to work on it. The other bug was removing the key, but as long as you put the key back in it will work.
Can this patch be applied to latest version of repo. The line numbers I see in the patch are not matching with what I am seeing the source files. Will that be any problem? Also what is left to Implement in this bug?
It probably can't be applied. There isn't really much left on the bug, it would just require applying the changes to the current code.
No longer blocks: 1257552
The caption content seems changed https://github.com/mozilla/gecko-dev/blob/master/devtools/client/shared/components/reps/array.js#L64 maybe we can pass > caption({ > key: "more", > msg: (array.length - max) + " more…" > }); in object, array, grip, grip-array and move the objectLink code in caption::render?
Flags: needinfo?(lclark)
The objectLink would have to be passed in too. I think that with the changes to the Caption, this doesn't make sense as an issue anymore.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(lclark)
Resolution: --- → INVALID
No longer blocks: devtools-html-2
Priority: P3 → --
Whiteboard: [reserve-html]
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: