Closed
Bug 1281206
Opened 8 years ago
Closed 8 years ago
Remove ObjectLink and ObjectBox reps
Categories
(DevTools :: Shared Components, defect, P1)
DevTools
Shared Components
Tracking
(firefox50 fixed)
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: linclark, Assigned: dalimil)
References
Details
(Whiteboard: [reserve-html])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•8 years ago
|
||
Honza, based on the above would you be ok with removing ObjectBox?
Flags: needinfo?(odvarko)
Comment 2•8 years ago
|
||
(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
Flags: needinfo?(odvarko)
Comment 3•8 years ago
|
||
Hi Lin, should this bug be added to the Track #2 backlog for the HTML Project?
Flags: needinfo?(lclark)
Updated•8 years ago
|
Updated•8 years ago
|
Priority: P2 → P3
Whiteboard: [devtools-html] → [reserve-html]
Reporter | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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)?
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(lclark)
Reporter | ||
Comment 7•8 years ago
|
||
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?
Flags: needinfo?(lclark)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dalimilhajek
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•8 years ago
|
||
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?
Flags: needinfo?(lclark)
Reporter | ||
Comment 9•8 years ago
|
||
Yes, for now let's keep the objectBox-* classes.
Flags: needinfo?(lclark)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Reporter | ||
Comment 11•8 years ago
|
||
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+
Assignee | ||
Comment 12•8 years ago
|
||
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
Assignee | ||
Comment 13•8 years ago
|
||
Fixed ESLint issues.
Push to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7663e5391868
Attachment #8776254 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/31f91a3da534
Remove ObjectLink and ObjectBox reps. r=lclark
Keywords: checkin-needed
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•