Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select component from dom tree #476

Merged
merged 19 commits into from
Jan 14, 2018
Merged

Select component from dom tree #476

merged 19 commits into from
Jan 14, 2018

Conversation

nicoeg
Copy link
Contributor

@nicoeg nicoeg commented Dec 20, 2017

Inspired by the react developer tools and of course Chromes developer tools.
This PR adds functionality for selecting a component from the dom tree.

Instead of just highlighting elements it highlights components themselves if the hovered element is not a Vue component it'll select the parent component.
The component tree will expand if the selected component is minimized.

dec-20-2017 22-39-20

Edit:
Just saw Zzuligy's PR.
While I think that its cool that he's using the developer tools selector I still feel that our PR's have some major differences. Eg. that my version will allow the feature to also function in Safari and Firefox.
I've messages Zzuligy to hear how he feels.

@Akryum
Copy link
Member

Akryum commented Dec 21, 2017

I see those PRs as complementary, why not having them both?

@nicoeg
Copy link
Contributor Author

nicoeg commented Dec 21, 2017

@Akryum
Yeah, that would be the best solution. I just thought since both our PR's are doing some of the same stuff like toggling all parent instances that we could agree on a way to do it so we don't have multiple methods in the codebase.

@nicoeg
Copy link
Contributor Author

nicoeg commented Jan 4, 2018

@Akryum I see you've added a better way to toggle parent instances in #479. Should I update this pr to follow that approach once yours is merged?

@nicoeg
Copy link
Contributor Author

nicoeg commented Jan 13, 2018

@Akryum Should be ready to review now

Rebased, updated to use new inspect-instance method and added tooltip to the button.
screen shot 2018-01-13 at 12 39 07

@Akryum
Copy link
Member

Akryum commented Jan 13, 2018

vue-devtools-highlighter

@Akryum Akryum requested review from yyx990803 and posva January 13, 2018 14:57
@yyx990803
Copy link
Member

Looks good to me!

@posva
Copy link
Member

posva commented Jan 13, 2018

I thought the right click extra item was enough, but displaying the name of the component is also really nice!

@Akryum
Copy link
Member

Akryum commented Jan 13, 2018

I thought the right click extra item was enough

Well, the browser offers both options. 😄

showOverlay(rect)
let content = ''
const name = getInstanceName(instance)
name && (content = `<span style="opacity: .6;">&lt;</span>${name}<span style="opacity: .6;">&gt;</span>`)
Copy link
Member

Choose a reason for hiding this comment

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

I would replace this with

if (name) {
	content = ...
	showOverlay(...)
}

since it doesn't look like it's displaying anything otherwise

Copy link
Member

Choose a reason for hiding this comment

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

It can still show the component boundaries even if the component doesn't have a name (most certainly using an old version of vue-loader or vue).

@@ -0,0 +1,6 @@
export function searchComponent (el) {
Copy link
Member

@posva posva Jan 13, 2018

Choose a reason for hiding this comment

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

I think we could have a more explicit name like findClosestWrappingComponent

Copy link
Member

Choose a reason for hiding this comment

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

We don't find the root component, we find the closest component that contains the element.

Copy link
Member

Choose a reason for hiding this comment

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

yes, sorry, I updated the comment right away 😆

@Akryum
Copy link
Member

Akryum commented Jan 13, 2018

@yyx990803 I think I'm done with this PR 😄

@Akryum
Copy link
Member

Akryum commented Jan 13, 2018

vue-devtools-select-tooltip

export const UP = 38
export const RIGHT = 39
export const DOWN = 40
export const S = 83
Copy link
Member

Choose a reason for hiding this comment

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

Do you also type in Dvorak? 😆
Using Keycodes doesn't work across keyboards for letters

Copy link
Member

Choose a reason for hiding this comment

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

Why? I just tried in Dvorak to be sure and it's working fine... 😕

Copy link
Member

Choose a reason for hiding this comment

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

You're completely right. I had recently a problem with keycodes but it's something a bit different, it only happens with the digits row

@yyx990803 yyx990803 mentioned this pull request Jan 14, 2018
16 tasks
@Akryum Akryum merged commit 1fe68b2 into vuejs:master Jan 14, 2018
@Akryum Akryum added this to the v4.1.0 milestone Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants