Skip to content
This repository was archived by the owner on Nov 25, 2021. It is now read-only.

Commit 3f92379

Browse files
sqssqs
committed
feat: provide a fetchActions instead of hard-coding "Go to def"
1 parent 08bdfc3 commit 3f92379

File tree

5 files changed

+136
-191
lines changed

5 files changed

+136
-191
lines changed

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,8 @@ Adds code intelligence to code views on the web. Used in [Sourcegraph](https://s
1111
## What it does
1212

1313
- Listens to hover and click events on the code view
14-
- On mouse hovers, determines the line+column position, does an LSP hover request and renders it in a nice tooltip overlay at the token
15-
- Shows button for go-to-definition
14+
- On mouse hovers, determines the line+column position, performs a hover request, and renders it in a nice tooltip overlay at the token
15+
- Shows actions in the hover
1616
- When clicking a token, pins the tooltip to that token
1717
- Highlights the hovered token
1818

‎src/HoverOverlay.tsx‎

Lines changed: 36 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -9,41 +9,21 @@ import { toNativeEvent } from './helpers'
99
import { HoveredToken } from './token_position'
1010
import { HoverAttachment, LOADING } from './types'
1111

12-
/** The component used to render a link */
13-
export type LinkComponent = React.ComponentType<{ to: string } & React.HTMLAttributes<HTMLElement>>
14-
1512
/**
16-
* Uses a placeholder `<button>` or a React Router `<Link>` depending on whether `to` is set.
13+
* The component used to render an action.
14+
*
15+
* @template A The type of an action.
1716
*/
18-
const ButtonOrLink: React.StatelessComponent<
19-
{ linkComponent: LinkComponent; to?: string } & React.HTMLAttributes<HTMLElement>
20-
> = ({ linkComponent, to, children, ...rest }) => {
21-
const Link = linkComponent
22-
return to ? (
23-
<Link to={to} {...rest}>
24-
{children}
25-
</Link>
26-
) : (
27-
<button {...rest}>{children}</button>
28-
)
29-
}
17+
export type ActionComponent<A> = React.ComponentType<A & React.HTMLAttributes<HTMLElement>>
3018

3119
/**
3220
* @template C Extra context for the hovered token.
21+
* @template A The type of an action.
3322
*/
34-
export interface HoverOverlayProps<C = {}> {
23+
export interface HoverOverlayProps<C extends object, A> {
3524
/** What to show as contents */
3625
hoverOrError?: typeof LOADING | HoverAttachment | null | ErrorLike // TODO disallow null and undefined
3726

38-
/**
39-
* The URL to jump to on go to definition.
40-
* If loaded, is set as the href of the go to definition button.
41-
* If LOADING, a loader is displayed on the button.
42-
* If null, an info alert is displayed "no definition found".
43-
* If an error, an error alert is displayed with the error message.
44-
*/
45-
definitionURLOrError?: typeof LOADING | { jumpURL: string } | null | ErrorLike
46-
4727
/** The position of the tooltip (assigned to `style`) */
4828
overlayPosition?: { left: number; top: number }
4929

@@ -62,36 +42,38 @@ export interface HoverOverlayProps<C = {}> {
6242
/** Whether to show the close button for the hover overlay */
6343
showCloseButton: boolean
6444

65-
/** The component used to render links */
66-
linkComponent: LinkComponent
45+
/**
46+
* Actions to display as buttons or links in the hover.
47+
*/
48+
actionsOrError?: typeof LOADING | A[] | null | ErrorLike
6749

6850
/** An optional class name to apply to the outermost element of the HoverOverlay */
6951
className?: string
7052

71-
/** Called when the Go-to-definition button was clicked */
72-
onGoToDefinitionClick?: (event: MouseEvent) => void
73-
7453
/** Called when the close button is clicked */
7554
onCloseButtonClick?: (event: MouseEvent) => void
7655
}
7756

78-
/** Returns true if the input is successful jump URL result */
79-
export const isJumpURL = (val: any): val is { jumpURL: string } =>
80-
val !== null && typeof val === 'object' && typeof val.jumpURL === 'string'
81-
8257
const transformMouseEvent = (handler: (event: MouseEvent) => void) => (event: React.MouseEvent<HTMLElement>) =>
8358
handler(toNativeEvent(event))
84-
85-
export const HoverOverlay: <C>(props: HoverOverlayProps<C>) => React.ReactElement<any> = ({
86-
definitionURLOrError,
59+
/**
60+
* @template C Extra context for the hovered token.
61+
* @template A The type of an action.
62+
*/
63+
export const HoverOverlay: <C extends object, A>(
64+
props: HoverOverlayProps<C, A> & {
65+
/** The component used to render actions. */
66+
actionComponent: ActionComponent<A>
67+
}
68+
) => React.ReactElement<any> = ({
8769
hoverOrError,
8870
hoverRef,
8971
children,
90-
linkComponent,
9172
onCloseButtonClick,
92-
onGoToDefinitionClick,
9373
overlayPosition,
9474
showCloseButton,
75+
actionsOrError,
76+
actionComponent: ActionComponent,
9577
className = '',
9678
}) => (
9779
<div
@@ -136,27 +118,24 @@ export const HoverOverlay: <C>(props: HoverOverlayProps<C>) => React.ReactElemen
136118
children
137119
)}
138120
</div>
139-
<div className="hover-overlay__actions hover-overlay__row">
140-
<ButtonOrLink
141-
linkComponent={linkComponent}
142-
to={isJumpURL(definitionURLOrError) ? definitionURLOrError.jumpURL : undefined}
143-
className="btn btn-secondary hover-overlay__action e2e-tooltip-j2d"
144-
onClick={onGoToDefinitionClick ? transformMouseEvent(onGoToDefinitionClick) : undefined}
145-
>
146-
Go to definition {definitionURLOrError === LOADING && <LoadingSpinner className="icon-inline" />}
147-
</ButtonOrLink>
148-
</div>
149-
{definitionURLOrError === null ? (
121+
{actionsOrError === null ? (
150122
<div className="alert alert-info hover-overlay__alert-below">
151123
<InformationOutlineIcon className="icon-inline" /> No definition found
152124
</div>
125+
) : isErrorLike(actionsOrError) ? (
126+
<div className="alert alert-danger hover-overlay__alert-below">
127+
<strong>
128+
<AlertCircleOutlineIcon className="icon-inline" /> Error finding definition:
129+
</strong>{' '}
130+
{upperFirst(actionsOrError.message)}
131+
</div>
153132
) : (
154-
isErrorLike(definitionURLOrError) && (
155-
<div className="alert alert-danger hover-overlay__alert-below">
156-
<strong>
157-
<AlertCircleOutlineIcon className="icon-inline" /> Error finding definition:
158-
</strong>{' '}
159-
{upperFirst(definitionURLOrError.message)}
133+
actionsOrError !== undefined &&
134+
actionsOrError !== LOADING && (
135+
<div className="hover-overlay__actions hover-overlay__row">
136+
{actionsOrError.map((action, i) => (
137+
<ActionComponent key={i} {...action} />
138+
))}
160139
</div>
161140
)
162141
)}

‎src/hoverifier.test.ts‎

Lines changed: 28 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import { EMPTY, NEVER, Observable, of, Subject, Subscription } from 'rxjs'
44
import { distinctUntilChanged, filter, map } from 'rxjs/operators'
55
import { TestScheduler } from 'rxjs/testing'
66

7-
import { noop } from 'lodash'
87
import { propertyIsDefined } from './helpers'
98
import {
109
AdjustmentDirection,
@@ -18,7 +17,7 @@ import {
1817
import { HoverOverlayProps } from './HoverOverlay'
1918
import { findPositionsFromEvents, SupportedMouseEvent } from './positions'
2019
import { CodeViewProps, DOM } from './testutils/dom'
21-
import { createHoverAttachment, createStubHoverFetcher, createStubJumpURLFetcher } from './testutils/lsp'
20+
import { createHoverAttachment, createStubActionsFetcher, createStubHoverFetcher } from './testutils/lsp'
2221
import { dispatchMouseEventAtPositionImpure } from './testutils/mouse'
2322
import { LOADING } from './types'
2423

@@ -46,8 +45,7 @@ describe('Hoverifier', () => {
4645
hoverOverlayElements: of(null),
4746
hoverOverlayRerenders: EMPTY,
4847
fetchHover: createStubHoverFetcher({ range: hoverRange }, LOADER_DELAY + delayTime),
49-
fetchJumpURL: () => of(null),
50-
pushHistory: noop,
48+
fetchActions: () => of(null),
5149
})
5250

5351
const positionJumps = new Subject<PositionJump>()
@@ -100,7 +98,6 @@ describe('Hoverifier', () => {
10098
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
10199

102100
const hover = {}
103-
const defURL = 'def url'
104101
const delayTime = 10
105102

106103
scheduler.run(({ cold, expectObservable }) => {
@@ -110,8 +107,7 @@ describe('Hoverifier', () => {
110107
hoverOverlayElements: of(null),
111108
hoverOverlayRerenders: EMPTY,
112109
fetchHover: createStubHoverFetcher(hover, delayTime),
113-
fetchJumpURL: createStubJumpURLFetcher(defURL, delayTime),
114-
pushHistory: noop,
110+
fetchActions: createStubActionsFetcher(['foo', 'bar'], delayTime),
115111
})
116112

117113
const positionJumps = new Subject<PositionJump>()
@@ -202,9 +198,10 @@ describe('Hoverifier', () => {
202198
// Only show on line 24, not line 25 (which is the 2nd click event below).
203199
fetchHover: position =>
204200
position.line === 24 ? createStubHoverFetcher({}, delayTime)(position) : of(null),
205-
fetchJumpURL: position =>
206-
position.line === 24 ? createStubJumpURLFetcher('def url', delayTime)(position) : of(null),
207-
pushHistory: noop,
201+
fetchActions: position =>
202+
position.line === 24
203+
? createStubActionsFetcher(['foo', 'bar'], delayTime)(position)
204+
: of(null),
208205
})
209206

210207
const positionJumps = new Subject<PositionJump>()
@@ -274,9 +271,8 @@ describe('Hoverifier', () => {
274271
// Only show on line 24, not line 25 (which is the 2nd click event below).
275272
fetchHover: position =>
276273
position.line === 24 ? createStubHoverFetcher({})(position) : of(null),
277-
fetchJumpURL: position =>
278-
position.line === 24 ? createStubJumpURLFetcher('def url')(position) : of(null),
279-
pushHistory: noop,
274+
fetchActions: position =>
275+
position.line === 24 ? createStubActionsFetcher(['foo', 'bar'])(position) : of(null),
280276
})
281277

282278
const positionJumps = new Subject<PositionJump>()
@@ -345,7 +341,7 @@ describe('Hoverifier', () => {
345341

346342
const delayTime = LOADER_DELAY + 100
347343
const hover = {}
348-
const defURL = 'def url'
344+
const actions = ['foo', 'bar']
349345

350346
scheduler.run(({ cold, expectObservable }) => {
351347
const hoverifier = createHoverifier({
@@ -354,8 +350,7 @@ describe('Hoverifier', () => {
354350
hoverOverlayElements: of(null),
355351
hoverOverlayRerenders: EMPTY,
356352
fetchHover: createStubHoverFetcher(hover, delayTime),
357-
fetchJumpURL: createStubJumpURLFetcher(defURL, delayTime),
358-
pushHistory: noop,
353+
fetchActions: createStubActionsFetcher(actions, delayTime),
359354
})
360355

361356
const positionJumps = new Subject<PositionJump>()
@@ -374,21 +369,18 @@ describe('Hoverifier', () => {
374369
})
375370
)
376371

377-
const hoverAndDefinitionUpdates = hoverifier.hoverStateUpdates.pipe(
372+
const hoverAndActionsUpdates = hoverifier.hoverStateUpdates.pipe(
378373
filter(propertyIsDefined('hoverOverlayProps')),
379-
map(({ hoverOverlayProps: { definitionURLOrError, hoverOrError } }) => ({
380-
definitionURLOrError,
374+
map(({ hoverOverlayProps: { actionsOrError, hoverOrError } }) => ({
375+
actionsOrError,
381376
hoverOrError,
382377
})),
383-
distinctUntilChanged(isEqual),
384-
// For this test, only emit when both hover and def are here.
385-
// Even though the fetchers are emitting at the same time, this observable emits twice.
378+
distinctUntilChanged((a, b) => isEqual(a, b)),
379+
// For this test, filter out the intermediate emissions where exactly one of the fetchers is
380+
// loading.
386381
filter(
387-
({ definitionURLOrError, hoverOrError }) =>
388-
!(
389-
(definitionURLOrError && hoverOrError === LOADING) ||
390-
(hoverOrError !== LOADING && !definitionURLOrError)
391-
)
382+
({ actionsOrError, hoverOrError }) =>
383+
(actionsOrError === LOADING) === (hoverOrError === LOADING)
392384
)
393385
)
394386

@@ -398,10 +390,10 @@ describe('Hoverifier', () => {
398390
const outputDiagram = `${LOADER_DELAY}ms a ${TOOLTIP_DISPLAY_DELAY - 1}ms b`
399391

400392
const outputValues: {
401-
[key: string]: Pick<HoverOverlayProps, 'hoverOrError' | 'definitionURLOrError'>
393+
[key: string]: Pick<HoverOverlayProps<{}, string>, 'hoverOrError' | 'actionsOrError'>
402394
} = {
403-
a: { hoverOrError: LOADING, definitionURLOrError: undefined }, // def url is undefined when it is loading
404-
b: { hoverOrError: createHoverAttachment(hover), definitionURLOrError: { jumpURL: defURL } },
395+
a: { hoverOrError: LOADING, actionsOrError: LOADING }, // actions is undefined when it is loading
396+
b: { hoverOrError: createHoverAttachment(hover), actionsOrError: actions },
405397
}
406398

407399
// Click https://sourcegraph.sgdev.org/github.com/gorilla/mux@cb4698366aa625048f3b815af6a0dea8aef9280a/-/blob/mux.go#L24:6
@@ -412,7 +404,7 @@ describe('Hoverifier', () => {
412404
})
413405
)
414406

415-
expectObservable(hoverAndDefinitionUpdates).toBe(outputDiagram, outputValues)
407+
expectObservable(hoverAndActionsUpdates).toBe(outputDiagram, outputValues)
416408
})
417409
}
418410
})
@@ -422,7 +414,6 @@ describe('Hoverifier', () => {
422414
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
423415

424416
const hover = {}
425-
const defURL = 'def url'
426417

427418
scheduler.run(({ cold, expectObservable }) => {
428419
const hoverifier = createHoverifier({
@@ -431,8 +422,7 @@ describe('Hoverifier', () => {
431422
hoverOverlayElements: of(null),
432423
hoverOverlayRerenders: EMPTY,
433424
fetchHover: createStubHoverFetcher(hover),
434-
fetchJumpURL: createStubJumpURLFetcher(defURL),
435-
pushHistory: noop,
425+
fetchActions: () => of(null),
436426
})
437427

438428
const positionJumps = new Subject<PositionJump>()
@@ -486,7 +476,6 @@ describe('Hoverifier', () => {
486476
const scheduler = new TestScheduler((a, b) => chai.assert.deepEqual(a, b))
487477

488478
const hover = {}
489-
const defURL = 'def url'
490479

491480
scheduler.run(({ cold, expectObservable }) => {
492481
const hoverifier = createHoverifier({
@@ -495,8 +484,7 @@ describe('Hoverifier', () => {
495484
hoverOverlayElements: of(null),
496485
hoverOverlayRerenders: EMPTY,
497486
fetchHover: createStubHoverFetcher(hover),
498-
fetchJumpURL: createStubJumpURLFetcher(defURL),
499-
pushHistory: noop,
487+
fetchActions: () => of(null),
500488
})
501489

502490
const positionJumps = new Subject<PositionJump>()
@@ -551,7 +539,7 @@ describe('Hoverifier', () => {
551539
/**
552540
* This test ensures that the adjustPosition options is being called in the ways we expect. This test is actually not the best way to ensure the feature
553541
* works as expected. This is a good example of a bad side effect of how the main `hoverifier.ts` file is too tightly integrated with itself. Ideally, I'd be able to assert
554-
* that the effected positions have actually been adjusted as intended but this is impossible with the current implementation. We can assert that the `HoverFetcher` and `JumpURLFetcher`s
542+
* that the effected positions have actually been adjusted as intended but this is impossible with the current implementation. We can assert that the `HoverFetcher` and `ActionsFetcher`s
555543
* have the adjusted positions (AdjustmentDirection.CodeViewToActual). However, we cannot reliably assert that the code "highlighting" the token has the position adjusted (AdjustmentDirection.ActualToCodeView).
556544
*/
557545
/**
@@ -565,7 +553,7 @@ describe('Hoverifier', () => {
565553
const adjustmentDirections = new Subject<AdjustmentDirection>()
566554

567555
const fetchHover = createStubHoverFetcher({})
568-
const fetchJumpURL = createStubJumpURLFetcher('def')
556+
const fetchActions = createStubActionsFetcher(['foo', 'bar'])
569557

570558
const adjustPosition: PositionAdjuster<{}> = ({ direction, position }) => {
571559
adjustmentDirections.next(direction)
@@ -579,8 +567,7 @@ describe('Hoverifier', () => {
579567
hoverOverlayElements: of(null),
580568
hoverOverlayRerenders: EMPTY,
581569
fetchHover,
582-
fetchJumpURL,
583-
pushHistory: noop,
570+
fetchActions,
584571
})
585572

586573
const positionJumps = new Subject<PositionJump>()

0 commit comments

Comments
 (0)