Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Highlight DOM Nodes onHover of Lexical DevTools Node #2728
Highlight DOM Nodes onHover of Lexical DevTools Node #2728
Changes from all commits
ca356ad
cc40d25
8c7f84c
ec0422d
591f478
d280ce0
f87197f
f29b06a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized a bit late that a lot of the stuff in
devtools.js
can be moved intocontent.js
. Particularly the parts that don't depend on theeditor
object, but are just declaring functions and variables in thewindow
scope.This would be ideal since they would be linted as TS in
content.ts
, and they wouldn't be run in aneval
.I can either move it in this PR with another commit, or early in the next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually forget about it, I spent the last couple of days working on this, but I can't get around the fact that
content.js
cannotdocument.querySelector
theeditorDOMNode
. This is because content scripts have limited access to the DOM.I don't think I'll be able to get around having the highlight/dehighlighting code within
devtools
.@thegreatercurve This is otherwise ready to merge!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been experimenting with some of the approaches in this StackOverflow post.
Some of it seems promising. In particular, I think the way to go in the future will be to have the content script inject something via
document.createElement('script')
. React DevTools takes this approach while still making limited use ofinspectedWindow.eval()
.I tried to get this approach running yesterday, but for some reason it doesn't quite work yet. I think it has something to do with timing, like the script is being injected before the DOM finishes loading. I'll keep experimenting for now, but I think whatever changes I make should be the subject of a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try defer loading of the script by setting
async
to betrue
on the script, or add an event listener on the window for after the document has fully loaded.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I tried some of these. I went with another workaround where the DevTools app sends a message, requesting to load the
editorState
the moment the DevTools panel is opened in the browser console. Seems to work fine.The good news is that I got all of the code out of the
eval()
statement and into an injected script, and it works great. I'll bundle the changes into another PR right after this one is merged.