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

Display a popover when hovering over highlighted mentions #6820

Merged
merged 1 commit into from
Feb 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/sidebar/components/Annotation/AnnotationBody.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ function AnnotationBody({ annotation, settings }: AnnotationBodyProps) {
const store = useSidebarStore();
const defaultAuthority = store.defaultAuthority();
const draft = store.getDraft(annotation);
const mentionsEnabled = store.isFeatureEnabled('at_mentions');

// If there is a draft use the tag and text from it.
const tags = draft?.tags ?? annotation.tags;
Expand Down Expand Up @@ -106,6 +107,7 @@ function AnnotationBody({ annotation, settings }: AnnotationBodyProps) {
})}
style={textStyle}
mentions={annotation.mentions}
mentionsEnabled={mentionsEnabled}
/>
</Excerpt>
)}
Expand Down
2 changes: 2 additions & 0 deletions src/sidebar/components/Annotation/test/AnnotationBody-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
} from '@hypothesis/frontend-testing';
import { mount } from '@hypothesis/frontend-testing';
import { act } from 'preact/test-utils';
import sinon from 'sinon';

import * as fixtures from '../../../test/annotation-fixtures';
import AnnotationBody, { $imports } from '../AnnotationBody';
Expand Down Expand Up @@ -54,6 +55,7 @@ describe('AnnotationBody', () => {
getLink: sinon
.stub()
.callsFake((linkPath, { tag }) => `http://www.example.com/${tag}`),
isFeatureEnabled: sinon.stub().returns(false),
};

$imports.$mock(mockImportedComponents());
Expand Down
1 change: 1 addition & 0 deletions src/sidebar/components/MarkdownEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -617,6 +617,7 @@ export default function MarkdownEditor({
classes="border bg-grey-1 p-2"
style={textStyle}
mentions={mentions}
mentionsEnabled={mentionsEnabled}
/>
) : (
<TextArea
Expand Down
132 changes: 121 additions & 11 deletions src/sidebar/components/MarkdownView.tsx
Original file line number Diff line number Diff line change
@@ -1,35 +1,102 @@
import { useEffect, useMemo, useRef } from 'preact/hooks';
import { Popover } from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import {
useCallback,
useEffect,
useMemo,
useRef,
useState,
} from 'preact/hooks';

import type { Mention } from '../../types/api';
import type { InvalidUsername } from '../helpers/mentions';
import { renderMentionTags } from '../helpers/mentions';
import { replaceLinksWithEmbeds } from '../media-embedder';
import { renderMathAndMarkdown } from '../render-markdown';
import StyledText from './StyledText';
import MentionPopoverContent from './mentions/MentionPopoverContent';

export type MarkdownViewProps = {
/** The string of markdown to display as HTML. */
markdown: string;
classes?: string;
style?: Record<string, string>;
mentions?: Mention[];

/**
* Whether the at-mentions feature ir enabled or not.
* Defaults to false.
*/
mentionsEnabled?: boolean;

// Test seams
setTimeout_?: typeof setTimeout;
clearTimeout_?: typeof clearTimeout;
};

type PopoverContent = Mention | InvalidUsername | null;

/**
* A component which renders markdown as HTML and replaces recognized links
* with embedded video/audio.
* A component which renders markdown as HTML, replaces recognized links with
* embedded video/audio and processes mention tags.
*/
export default function MarkdownView({
markdown,
classes,
style,
mentions,
acelaya marked this conversation as resolved.
Show resolved Hide resolved
}: MarkdownViewProps) {
export default function MarkdownView(props: MarkdownViewProps) {
/* istanbul ignore next - Unpack here to ignore default values for test seams */
const {
markdown,
classes,
style,
mentions = [],
mentionsEnabled = false,
setTimeout_ = setTimeout,
clearTimeout_ = clearTimeout,
} = props;
const html = useMemo(
() => (markdown ? renderMathAndMarkdown(markdown) : ''),
[markdown],
);
const content = useRef<HTMLDivElement | null>(null);

const mentionsPopoverAnchorRef = useRef<HTMLElement | null>(null);
const elementToMentionMap = useRef(
new Map<HTMLElement, Mention | InvalidUsername>(),
);
const [popoverContent, setPopoverContent] = useState<PopoverContent>(null);
const popoverContentTimeout = useRef<ReturnType<typeof setTimeout> | null>();
const setPopoverContentAfterDelay = useCallback(
// This allows the content to be set with a small delay, so that popovers
// don't flicker simply by hovering an annotation with mentions
(content: PopoverContent) => {
if (popoverContentTimeout.current) {
clearTimeout_(popoverContentTimeout.current);
}

const setContent = () => {
setPopoverContent(content);
popoverContentTimeout.current = null;
};

// Set the content immediately when resetting, so that there's no delay
// when hiding the popover, only when showing it
if (content === null) {
setContent();
} else {
popoverContentTimeout.current = setTimeout_(setContent, 400);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Popovers appear after hovering for 400ms. I compared the time it takes in GitHub, and this is the value I came up "by eye", because I wasn't able to find a way to properly measure it.

}
},
[clearTimeout_, setTimeout_],
);

useEffect(() => {
// Clear any potentially in-progress popover timeout when this component is
// unmounted
return () => {
if (popoverContentTimeout.current) {
clearTimeout_(popoverContentTimeout.current);
}
};
}, [clearTimeout_]);

useEffect(() => {
replaceLinksWithEmbeds(content.current!, {
// Make embeds the full width of the sidebar, unless the sidebar has been
Expand All @@ -40,7 +107,7 @@ export default function MarkdownView({
}, [markdown]);

useEffect(() => {
renderMentionTags(content.current!, mentions ?? []);
elementToMentionMap.current = renderMentionTags(content.current!, mentions);
}, [mentions]);

// NB: The following could be implemented by setting attribute props directly
Expand All @@ -50,14 +117,57 @@ export default function MarkdownView({
// a review in the future.
return (
<div className="w-full break-anywhere cursor-text">
<StyledText>
<StyledText
classes={classnames({
// A `relative` wrapper around the `Popover` component is needed for
// when the native Popover API is not supported.
relative: mentionsEnabled,
})}
>
<div
className={classes}
data-testid="markdown-text"
ref={content}
dangerouslySetInnerHTML={{ __html: html }}
style={style}
// React types do not define `onMouseEnterCapture`, but preact does
// eslint-disable-next-line react/no-unknown-property
Copy link
Contributor Author

@acelaya acelaya Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling react/no-unknown-property here is needed because it evaluates props based on react types, not preact ones, where the Capture variants of event handlers are not defined.

We discussed in slack the possibility of completely disabling react/no-unknown-property, since TypeScript covers the same, more accurately, but I will do that on a PR of its own.

onMouseEnterCapture={
mentionsEnabled
? ({ target }) => {
const element = target as HTMLElement;
const mention = elementToMentionMap.current.get(element);

if (mention) {
setPopoverContentAfterDelay(mention);
mentionsPopoverAnchorRef.current = element;
}
}
: undefined
}
// React types do not define `onMouseLeaveCapture`, but preact does
// eslint-disable-next-line react/no-unknown-property
onMouseLeaveCapture={
mentionsEnabled
? () => {
setPopoverContentAfterDelay(null);
mentionsPopoverAnchorRef.current = null;
}
: undefined
}
/>
{mentionsEnabled && (
<Popover
open={!!popoverContent}
onClose={() => setPopoverContentAfterDelay(null)}
anchorElementRef={mentionsPopoverAnchorRef}
classes="px-3 py-2"
>
{popoverContent !== null && (
<MentionPopoverContent content={popoverContent} />
)}
</Popover>
)}
</StyledText>
</div>
);
Expand Down
35 changes: 35 additions & 0 deletions src/sidebar/components/mentions/MentionPopoverContent.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import type { Mention } from '../../../types/api';
import type { InvalidUsername } from '../../helpers/mentions';

export type MentionPopoverContent = {
content: Mention | InvalidUsername;
};

/**
* Information to display in a Popover when hovering over a processed mention.
*/
export default function MentionPopoverContent({
content,
}: MentionPopoverContent) {
if (typeof content === 'string') {
return (
<>
No user with username <span className="font-bold">{content}</span>{' '}
exists
</>
);
}

return (
<div className="flex flex-col gap-y-1.5">
<div data-testid="username" className="text-md font-bold">
@{content.username}
</div>
{content.display_name && (
<div data-testid="display-name" className="text-color-text-light">
{content.display_name}
</div>
)}
</div>
);
}
37 changes: 37 additions & 0 deletions src/sidebar/components/mentions/test/MentionPopoverContent-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { mount } from '@hypothesis/frontend-testing';

import MentionPopoverContent from '../MentionPopoverContent';

describe('MentionPopoverContent', () => {
function createComponent(content) {
return mount(<MentionPopoverContent content={content} />);
}

it('renders user-not-found message when InvalidUser is provided', () => {
const wrapper = createComponent('@invalid');

assert.equal('No user with username @invalid exists', wrapper.text());
assert.isFalse(wrapper.exists('[data-testid="username"]'));
assert.isFalse(wrapper.exists('[data-testid="display-name"]'));
});

it('renders username when valid mention without display name is provided', () => {
const wrapper = createComponent({ username: 'janedoe' });

assert.equal(wrapper.find('[data-testid="username"]').text(), '@janedoe');
assert.isFalse(wrapper.exists('[data-testid="display-name"]'));
});

it('renders username and display name when valid mention with display name is provided', () => {
const wrapper = createComponent({
username: 'janedoe',
display_name: 'Jane Doe',
});

assert.equal(wrapper.find('[data-testid="username"]').text(), '@janedoe');
assert.equal(
wrapper.find('[data-testid="display-name"]').text(),
'Jane Doe',
);
});
});
Loading