Closed Bug 1335050 Opened 3 years ago Closed 3 years ago

Allow customization of object rep's title generation.

Categories

(DevTools :: Shared Components, defect)

defect
Not set

Tracking

(firefox54 fixed)

RESOLVED FIXED
Firefox 54
Tracking Status
firefox54 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

Attachments

(2 files, 1 obsolete file)

At the moment reps/object's getTitle method is too opinionated where title depends on on whether the object has class property. In some cases it is not desired so it would be nice to be able to pass getTitle as part of props.
Attached patch 1335050 patch (obsolete) — Splinter Review
Attachment #8831707 - Flags: review?(odvarko)
Component: Developer Tools → Developer Tools: Shared Components
I like the patch.  Do you have any use case for the new property? Shouldn't we do it for all reps?

Honza
Flags: needinfo?(yzenevich)
I am also CCing Julian here.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #2)
> I like the patch.  Do you have any use case for the new property? Shouldn't
> we do it for all reps?
> 
> Honza

Hi Jan, so my use case is for the work (still not in any of the review queues) that I'm doing for accessibility inspection. I am rendering a tree with accessibility object properties where one of them is called attributes. They correspond to what accessibility module calculates for any particular accessible object. One of the attribute names that it exposes is "class". So what happens is when I want to use an Obj rep, it uses "class" attribute's value as the title. That is however confusing since non of the accessibility attributes have any priority or elevated importance that could be used as a title. What my override getTitle prop does is returning "Object" string (similar to what obj does by default when class is not present). Hope this is enough context.

Not sure what other ones you have in mind, my guess is that it could be useful for other opinionated ones, but I, so far, haven't seen any issues in my use cases other than Obj.
Flags: needinfo?(yzenevich)
Just a heads up: the reps are in the process of being moved to Github. I should complete the migration this week. 

Feel free to finish the development/review here, I'll port the development to Github in case this doesn't land before the migration.
Blocks: 1330779
:yzen, do note that we already do such thing in the Grip reps (https://github.com/devtools-html/devtools-reps/blob/master/src/reps/grip.js#L33), where we only pass a string, and not a function. 
Like Honza said, it would be really nice to have such mechanism for all Reps :)
@yzen: would it be enough for you to use the same approach as Grip rep? (see comment #6)

So, getting the title would look like as follows:
let title = this.props.title || object.class || "Object";

... so, you can just pass an extra 'title' prop to customize the title.

This way the implementation would be consistent between Grip and Obj reps.

Honza
Flags: needinfo?(yzenevich)
Attached patch 1335050 patch v2Splinter Review
Making implementation consistent between Grip and Obj
Attachment #8831707 - Attachment is obsolete: true
Attachment #8831707 - Flags: review?(odvarko)
Flags: needinfo?(yzenevich)
Attachment #8834748 - Flags: review?(odvarko)
Comment on attachment 8834748 [details] [diff] [review]
1335050 patch v2

Review of attachment 8834748 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

R+ assuming Try is green (note that the condition checking whether the `object` is null is gone now)

Honza
Attachment #8834748 - Flags: review?(odvarko) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb66941fca47
Allow customization of object rep's title generation. r=Honza
https://hg.mozilla.org/mozilla-central/rev/fb66941fca47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
https://hg.mozilla.org/mozilla-central/diff/fb66941fca47/git is an empty file. Is this intentional?
Flags: needinfo?(yzenevich)
Attached patch 1335050 clean upSplinter Review
My bad! my git-hg tool added this somehow. Here's the patch to get rid of it.
Flags: needinfo?(yzenevich)
Attachment #8837608 - Flags: review?(odvarko)
Comment on attachment 8837608 [details] [diff] [review]
1335050 clean up

Thanks for working on this!

Honza
Attachment #8837608 - Flags: review?(odvarko) → review+
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e74396a79be
cleaning up acceidentally committed file. r=Honza
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.