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

Configure window reps title #73

Merged
merged 4 commits into from
Feb 11, 2017

Conversation

juliandescottes
Copy link
Member

This is an attempt at fixing #47

Instead of introducing the debugger.html specific isGlobal check, I wanted to reuse the "title" property already used in a few reps.

This made me realise that we had a bug with grip and grip-array reps which were copying their title prop to their children. The first commit fixes that (as well as updating a test that relied on the bug).

The second commit adds the title prop to the window rep, as well as a TINY render mode. This means that now window objects will be rendered as Window (or their customized title) when they are rendered in TINY mode.

Comparing with the old console, this is a behaviour change, as the old console always displayed Window -> url it seems. I think this is an acceptable change though (and Chrome only displays Window for instance).

I'll wait for your feedback before adding tests dedicated to that.

@juliandescottes
Copy link
Member Author

cc @nchevobbe

@nchevobbe nchevobbe self-requested a review February 11, 2017 13:15
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.

I think this commit (d91aeaa) shouldn't be part of this PR ?

@juliandescottes
Copy link
Member Author

I think this commit (d91aeaa) shouldn't be part of this PR ?

Sure, doesn't prevent us from discussing though :)

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.

it looks good, thanks for the fix :)

@juliandescottes
Copy link
Member Author

Thanks for the feedback @nchevobbe !
Added some tests in my last iteration. Feel free to have a look while we wait for CI :)

@juliandescottes
Copy link
Member Author

(actually I'll wait for you to merge your big PR, and I'll rebase on top of it)

@nchevobbe
Copy link
Member

it's merged now :)

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 test seems good :)
Would you mind adding some test to make sure we don't pass the title props to children when we don't need, fro grip and grip-array ?

@juliandescottes
Copy link
Member Author

I added some simple tests in the existing test file for grip, grip-array and grip-map. I also added support for custom titles for grip array and grip maps, otherwise the tests were a little weird (we were defining a custom title that wasn't visible anywhere in the output)

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.

great 👍

@nchevobbe nchevobbe merged commit 42649d0 into firefox-devtools:master Feb 11, 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