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

Add syntax highlighting to conditional panel #7911

Merged
merged 5 commits into from Feb 11, 2019

Conversation

@anthonyx
Copy link
Contributor

commented Feb 7, 2019

Fixes #7800

Please let me know your thoughts and what can be improved. Thank you!

Summary of Changes

  • Used CodeMirror to implement syntax highlighting for the conditional panel. This affects both conditional breakpoints and conditional log points.
  • Deleted code for placeholder/keyDown attributes on the input since the functionality is replaced by the CodeMirror instance

Test Plan

[x] Added conditional breakpoint condition and saw syntax highlighting was in effect
[x] Edited conditional breakpoint condition and saw syntax highlighting was in effect
[x] Ensured code did not pause when condition was not met
[x] Ensured code paused when condition was met
[x] Removed conditional breakpoint

Demo

feb-07-2019 14-43-09

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

This is...wow!

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

Travis is bombing because of the following error upon opening the debugger:

JavaScript error: resource://devtools/shared/base-loader.js, line 195: Error: Module `codemirror/index.js` is not found at codemirror/index.js
console.error: (new TypeError("this.panelWin.Debugger is undefined", "resource://devtools/client/debugger/new/panel.js", 36))

Looking into it.

@darkwing

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

@ravefalcon92 It turns out we need to use SourceEditor (src/utils/editor/source-editor.js) to get CodeMirror working properly in built firefox -- could you look at that and see if you could switch to that API? It's not too different :) 👍

@darkwing darkwing added the 📚 pr-wip label Feb 8, 2019

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

i pushed some tweaks to get the tests to pass. Great work!

@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Feb 9, 2019

One challenge i found was supporting horizontal scrolling. ideally the input remains fixed.

by the way, it might help to temporarily comment out the onBlur code so you can inspect it without it going away...

jasonLaster and others added some commits Feb 9, 2019

@anthonyx

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2019

@jasonLaster, thank you for tweaking the code to pass the tests!
I've edited the code so that the input stays fixed when scrolling horizontally.

horizontal_scroll

btw, thanks @darkwing for this post, it was helpful for this PR!

Let me know if there's anything else that needs to be fixed or can be improved.

@jasonLaster jasonLaster merged commit 6b0b968 into firefox-devtools:master Feb 11, 2019

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jasonLaster

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2019

Nope. It works great. I'm thrilled to land this along side log points #7811. By the way, if you have time i'd really appreciate if you could pull #7811 down and test it :)

@anthonyx anthonyx deleted the anthonyx:syntax-highlight-conditional-panel branch Feb 11, 2019

@anthonyx

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

Sounds good, I'll take a look and see if I can help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.