Expose the currently hovered token and support disabling pinning #106
Conversation
chrismwendt
force-pushed
the
single-click-j2d
branch
from
81fbc26 to
8a5ec31
Compare
Codecov Report
@@ Coverage Diff @@
## master #106 +/- ##
==========================================
+ Coverage 84% 84.15% +0.15%
==========================================
Files 13 13
Lines 525 530 +5
Branches 120 123 +3
==========================================
+ Hits 441 446 +5
Misses 84 84
Continue to review full report at Codecov.
|
chrismwendt
force-pushed
the
single-click-j2d
branch
from
8a5ec31 to
fdd3238
Compare
chrismwendt
force-pushed
the
single-click-j2d
branch
from
fdd3238 to
f1142c5
Compare
|
Added some tests. @lguychard Did you want to do another review? |
| @@ -226,6 +241,9 @@ interface InternalHoverifierState<C extends object, D, A> { | |||
| /** The currently hovered token */ | |||
| hoveredToken?: HoveredToken & C | |||
|
|
|||
| /** The currently hovered token HTML element */ | |||
| hoveredTokenElement?: HTMLElement | |||
There was a problem hiding this comment.
The hover state is supposed to be props to be passed into a React component, so it seems weird to have an HTMLElement in it. The PR description doesn't contain any explanation for it, could you explain why this is needed?
There is also no guarantee that a "token" is its own HTML element. We actually want to move away from DOM mutation to wrap tokens in elements, so this would make it impossible to do that or the hoveredTokenElement would not only correspond to the hovered token but be an arbitrary element.
Another not nice thing is that the addition of this property allows for invalid state in this interface (hoveredToken can be set while hoveredTokenElement is not, where in reality that is an invalid state). This means it's easy to forget to reset one or the other in a call to container.update() and cause bugs.
Another minor concern is that currently all the properties in here are immune to the code host rerendering or mutating elements in the code view, but this element would maybe not be attached to the DOM anymore but then still be in the state and used by the consumer.
There was a problem hiding this comment.
The only reason to expose hoveredTokenElement is so that the Sourcegraph web app can set CSS properties and add a click listener to it https://github.com/sourcegraph/sourcegraph/pull/3444/files#diff-159ed2efd1fa050e21aa970011528ceeR224
I'm not very familiar with the codeintellify API. Is there a way to convert a HoveredToken into an HTML element? If so, this hoveredTokenElement field can be dropped. I think that would completely solve paragraphs 2 and 4.
If/when we move away from DOM mutation to wrap tokens in elements, couldn't we apply whatever technique we come up with for solving the problem of token highlighting to the problem of setting cursor: pointer and adding a click listener? I suppose it depends on the specific technique, but hypothetically if we overlay semi-transparent spans, I think it could work.
None of your concerns block merging of this PR, do they? This PR doesn't affect existing behavior. It would be great to be able to both experiment now AND improve the code asynchronously (best of both worlds).
| /** | ||
| * Whether or not hover tooltips can be pinned. | ||
| */ | ||
| pinningEnabled: boolean |
There was a problem hiding this comment.
Consider that a hoverifier is a single instance per page, and the setting for this can change during the life time of it
There was a problem hiding this comment.
The global/org/user setting singleClickGoToDefinition? I think it's fine to require a page reload to pick up the setting (the same applies to all settings changes).
|
Merging now to start experimenting. I'm happy to continue the review process post-merge. |
|
🎉 This PR is included in version 6.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
I see the tracking issue at https://github.com/sourcegraph/sourcegraph/issues/3449 considers this feature complete. When are you planning to address the review feedback? I don't feel comfortable with maintaining this code long-term. |
|
First, let's decide whether or not to keep it (separate discussion). If there's any feedback that I didn't address, let's discuss that here and I'll include it in a separate PR to update the browser extension. |
This will let Sourcegraph provide single-click jump-to-definition behavior:
Subsumes #42 cc @dadlerj just FYI