1281206 - Remove ObjectLink and ObjectBox reps

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

Discussed with Honza at the all hands and in IRC today.

We definitely want to remove ObjectLink. We're planning to allow the parent component to pass in a component to be used for linking.

Removing ObjectBox isn't as clear, but I believe we should do it. Currently it just serves as a place to put a class name, but a div can do this in the same way.

The argument for keeping it is that it might be useful in the future as a place to hang things like contextual menus.

However, for now it results in a small performance hit (about 5-7% increase in mountComponent in my back of the napkin performance tests). It also adds an abstraction to the reps system that is currently unnecessary, and which I found confusing when I first approached the API. And once we get to implementing contextual menus, we may find that this is not the approach we want to use.

Since it's not providing a real benefit at this point, I'd say we should remove it too. We can reintroduce it later if we have a use for it.

Comment 1

[Lin Clark [:linclark]]

Honza, based on the above would you be ok with removing ObjectBox?

Comment 2

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

(In reply to Lin Clark [:linclark] from comment #1)
> Honza, based on the above would you be ok with removing ObjectBox?
Ok, you convinced me.

Honza

Comment 3

[Marco Mucci [:MarcoM]]

Hi Lin, should this bug be added to the Track #2 backlog for the HTML Project?

Comment 4

[Lin Clark [:linclark]]

Yes, that makes sense

Comment 5

[Lin Clark [:linclark]]

There is now both an objectLink property and and ObjectLink component. We want to remove the ObjectLink component, not the objectLink property and its related code.

Comment 6

[Dalimil Hajek [:dalimil]]

Attribute.js seems to be the only component directly using object-link.js - are we going to be passing the ObjectLink to Attribute via this.props.objectLink (for example as in object.js)?

Comment 7

[Lin Clark [:linclark]]

I believe that attribute.js should be using ObjectBox, not ObjectLink. It may have just been missed when we did the pass through. However, it should also have the objectLink property handling logic that is in the rest of the reps. Could you take care of that as part of this issue?

Comment 8

[Dalimil Hajek [:dalimil]]

There are quite a few rules in reps.css for various data types: "objectBox-"+className and also "objectLink-"+className 
How should I treat those? Do you want me to keep the objectBox-* classes for the new span elements replacing the ObjectBox rep?

Comment 9

[Lin Clark [:linclark]]

Yes, for now let's keep the objectBox-* classes.

Comment 10

[Dalimil Hajek [:dalimil]]

Attachment: Bug1281206.patch - rev1

So I removed the ObjectBox and ObjectLink reps. See the code changes and tell me what you think...

I only tested locally so far - the only test that is failing is for the Undefined rep. ->   is(renderedComponent.getAttribute("role"), "presentation", "Undefined rep has expected aria role");

I think that's because I didn't add the 'role' parameter...

I'm not sure how the 'role': "presentation" parameter is used. Should I attach it to every span element? ( Same as with the 'objectBox-*' classes)

Comment 11

[Lin Clark [:linclark]]

Comment on attachment 8775169 [details] [diff] [review]
Bug1281206.patch - rev1

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

This looks great to me, thanks! We are going to remove the presentation role anyway in Bug 1272364, so you can just remove that from the test.

If this passes on try, then it will be ready for checkin-needed

Comment 12

[Dalimil Hajek [:dalimil]]

Attachment: Bug1281206.patch - rev2

I removed that single test case and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f14322dba1

Comment 13

[Dalimil Hajek [:dalimil]]

Attachment: Bug1281206.patch - rev3

Fixed ESLint issues.
Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7663e5391868

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/31f91a3da534
Remove ObjectLink and ObjectBox reps. r=lclark

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/31f91a3da534