1335050 - Allow customization of object rep's title generation.

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1335050 patch

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I like the patch.  Do you have any use case for the new property? Shouldn't we do it for all reps?

Honza

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I am also CCing Julian here.

Honza

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

(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.

Comment 5

[Julian Descottes [:jdescottes]]

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.

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

: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 :)

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

@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

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1335050 patch v2

Making implementation consistent between Grip and Obj

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb66941fca47
Allow customization of object rep's title generation. r=Honza

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fb66941fca47

Comment 12

[Jan Beich]

https://hg.mozilla.org/mozilla-central/diff/fb66941fca47/git is an empty file. Is this intentional?

Comment 13

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: 1335050 clean up

My bad! my git-hg tool added this somehow. Here's the patch to get rid of it.

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8837608 [details] [diff] [review]
1335050 clean up

Thanks for working on this!

Honza

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e74396a79be
cleaning up acceidentally committed file. r=Honza

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e74396a79be