Skip to content
This repository has been archived by the owner on May 2, 2018. It is now read-only.

allow window rep title customization from grip #74

Merged
merged 1 commit into from
Feb 14, 2017

Conversation

juliandescottes
Copy link
Member

@juliandescottes juliandescottes commented Feb 13, 2017

cc @nchevobbe

My previous attempt (PR #73 ) actually doesn't address the debugger needs. They need to be able to configure the window rep title from the grip itself and not from the rep.

Looking at it more carefully I guess it makes more sense to make the title of this rep customizable based on the grip rather than on the rep's props.

So here I'm using an additional property displayClass in order to display a custom title for a window rep (to be consistent with class which is usually used as the title for the window rep).

I could re-add the option to also define this.props.title, but in this case we need to discuss the priority of this.props.title vs object.displayClass.

IMO it should be object.displayClass || this.props.title || object.class || "Window" ( but again I doubt a title at reps level here makes a lot of sense)

Copy link
Member

@nchevobbe nchevobbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me.
What I don't really understand is how adding a displayClass property to the grip is different than passing a title as a prop to the Rep ?
Will the displayClass will be send by the server or will the debugger add it ?

@juliandescottes
Copy link
Member Author

The debugger will decorate the grip with this additional property (same as what they did before with the boolean : https://github.com/devtools-html/debugger.html/blob/42f7c52c26a41ddc3cc8856f78a2aca315940c8d/src/components/SecondaryPanes/Scopes.js#L125).

Looking at the code, it is easy for the debugger to inject new information in the object, but then they use the ObjectInspector to create the Rep()s itself. If the debugger wants to set the title as a rep property, it needs to be done in the ObjectInspector.

I don't think the ObjectInspector should embed the logic to display "Global" instead of "Window" and I can't think of a clean API that would allow the caller of the ObjectInspector to pass the necessary information.

Having a grip-level property seems like the best fit here IMO.

@nchevobbe
Copy link
Member

Thanks for the explanation Julian. I now understand how this could be better.
What I wonder now is if we should have the same thing for the other Reps that can have a custom title :)

Let's merge this and open an issue to have consistency for those titles across the reps

@nchevobbe nchevobbe merged commit 5aa76f5 into firefox-devtools:master Feb 14, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants