Remove column breakpoint#3120
Conversation
|
Here's another URL for testing: http://localhost:8000/integration/examples/doc-sourcemaps2.html |
b6196ec to
e821a7a
Compare
Codecov Report
@@ Coverage Diff @@
## master #3120 +/- ##
==========================================
- Coverage 47.51% 47.44% -0.08%
==========================================
Files 95 95
Lines 4003 4009 +6
Branches 821 823 +2
==========================================
Hits 1902 1902
- Misses 2101 2107 +6
Continue to review full report at Codecov.
|
|
Hey @anbarasiu! thanks so much for the help, this PR actually scared me for much of the day because sliding and bps are hard! I made some small tweaks because i found the ternary boolean hard to read. I tried to expand it out a bit for my future self and others. I genuinely cannot wait for column breakpoints to land. They'll be so great! |
fbc73b5 to
f9880d0
Compare
|
Thanks for your help :) Looked at the changes and I'll keep the readability in mind for the next PR. I've made a small change, so that clicked bp is returned.
Was wondering if it would be better to remove the bp, not just when clicking on the gutter, but also the bp itself, when column > 0, like in the gif. |
|
|
||
| if (bp) { | ||
| // NOTE: it's possible the breakpoint has slid to a column | ||
| column = column || bp.location.column; |
There was a problem hiding this comment.
I am wondering which one should take precedence here. If both column and bp.location.column are defined, which one would be more important? From the comment it sounds like bp.location.column -- because it should be overwriting to account for sliding? But I'm not too sure, sliding bps are hard!
There was a problem hiding this comment.
Hmm.. I think column should take precedence, so that only when column info's not available we pick it up from bp. When initiated on the gutter, the column is undefined(from what I observed). Perhaps, when we directly click on the bp it could be passed(from codemirror?)?
Please correct me if I'm wrong here :)
There was a problem hiding this comment.
Ah ok i see what is going on here. I think the comment should be more specific, maybe something like:
// falling back on the bp.location.column allows toggling the breakpoint a from the gutter, even if it is a column breakpoint
thats... probably a bit verbose. might be a better way to word what is going on here
There was a problem hiding this comment.
ugh, this is an evil case. if we're clicking on a column breakpoint then we want to remove the breakpoint on that column. but we can cross that bridge when we get there
|
|
||
| // NOTE: when column breakpoints are disabled we want to find | ||
| // the first breakpoint | ||
| if (!isEnabled("columnBreakpoints") || !column) { |
efb97e0 to
9193907
Compare
Associated Issue: #3042
Summary of Changes
Test Plan