Closed
Bug 1276210
Opened 8 years ago
Closed 8 years ago
Reps: Use getDefaultProps for Caption object
Categories
(DevTools :: Shared Components, defect)
DevTools
Shared Components
Tracking
(firefox49 affected)
RESOLVED
INVALID
Tracking | Status | |
---|---|---|
firefox49 | --- | affected |
People
(Reporter: linclark, Unassigned)
References
Details
Attachments
(1 file)
(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..."
> };
> }
Reporter | ||
Comment 1•8 years ago
|
||
Comment 2•8 years ago
|
||
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+
Reporter | ||
Updated•8 years ago
|
Whiteboard: [devtools-html] [triage]
Updated•8 years ago
|
Blocks: 1257552, devtools-html-2
Iteration: --- → 49.3 - Jun 6
Flags: qe-verify-
Priority: -- → P1
Whiteboard: [devtools-html] [triage] → [devtools-html]
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Updated•8 years ago
|
Assignee: lclark → nobody
Status: ASSIGNED → NEW
Iteration: 50.1 → ---
Priority: P1 → P2
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Comment 3•8 years ago
|
||
I saw the patch is r+ and not checked-in-ed. Any related work needed before check in the patch?
Flags: needinfo?(lclark)
Reporter | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
Is this assigned? Or can I take it?
Reporter | ||
Comment 6•8 years ago
|
||
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"}));
Comment 7•8 years ago
|
||
(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 ?
Reporter | ||
Comment 8•8 years ago
|
||
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.
Comment 9•8 years ago
|
||
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?
Reporter | ||
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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)
Reporter | ||
Comment 12•8 years ago
|
||
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
Updated•8 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•