Closed Bug 1281206 Opened 3 years ago Closed 3 years ago
Link and Object Box reps
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.
Honza, based on the above would you be ok with removing ObjectBox?
(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
Hi Lin, should this bug be added to the Track #2 backlog for the HTML Project?
Yes, that makes sense
Priority: -- → P2
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
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.
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)?
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?
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?
Yes, for now let's keep the objectBox-* classes.
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)
Attachment #8775169 - Flags: review?(lclark)
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
Attachment #8775169 - Flags: review?(lclark) → review+
I removed that single test case and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=05f14322dba1
Attachment #8775169 - Attachment is obsolete: true
Fixed ESLint issues. Push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7663e5391868
Attachment #8776254 - Attachment is obsolete: true
Pushed by email@example.com: https://hg.mozilla.org/integration/fx-team/rev/31f91a3da534 Remove ObjectLink and ObjectBox reps. r=lclark
You need to log in before you can comment on or make changes to this bug.