Positions come from outside of hoverifier #9
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 51.94% 52.33% +0.39%
==========================================
Files 14 15 +1
Lines 541 577 +36
Branches 134 138 +4
==========================================
+ Hits 281 302 +21
- Misses 260 275 +15
Continue to review full report at Codecov.
|
| hoverOverlayRerenders: Observable<{ | ||
| hoverOverlayElement: HTMLElement | ||
| scrollElement: HTMLElement | ||
| }> |
There was a problem hiding this comment.
where do these changes come from?
There was a problem hiding this comment.
cmd+s in vscode... Prettier doing its thing but not sure why to be honest
This operator doesn't need all the local variables, they are all just used once. (The share() is unneeded too)
PositionEvent already includes the codeElement
|
@ijsnow any more cleanup you want to do here? |
|
Did you run it after your changes? It doesn't work anymore. |
|
No, I thought my changes were noops. Do you know which specific change? |
|
So there are two bugs in this PR, one was caused by your changes to my changes, the other was caused by my original PR it seems.
Essentially there is some race condition caused by the mouseover and click events along with the
|
That's weird cause it wasn't part of |
|
I believe it's because the event operator just emits the positions of events and doesn't do any kind of custom filtering. That's how I think it should be. It's possible that we lost some filtering along the way. If you run the code, you'll see the bug I'm talking about and if you add those back in, you'll see that the problem is fixed. |
felixfbecker
force-pushed
the
external-positions
branch
from
5b0dd35 to
23d9f34
Compare
| // TODO locateTarget is purely needed here to get `hoveredToken.part` for diffs | ||
| // We should define a function that takes care of _only_ figuring out the `part` | ||
| // so we don't have to use locateTarget | ||
| const hoveredToken = locateTarget(target, codeElement, false) |
|
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This PR moves the position finding for events outside of hoverifier. This is part of the steps for modularizing this code base.. This will close #4.
It was tested in
sourcegraph/sourcegraphand everything works as expected.