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 description and joining date in mentioned user popover #6824

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Feb 13, 2025

Closes #6822

Depends on hypothesis/h#9349, hypothesis/h#9352 and hypothesis/frontend-shared#1879

Display the user description and the date in which they joined in the mention popover.

image

Tip

Easier to review ignoring whitespaces

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.42%. Comparing base (386628e) to head (3557f2d).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6824   +/-   ##
=======================================
  Coverage   99.42%   99.42%           
=======================================
  Files         272      272           
  Lines       10446    10448    +2     
  Branches     2507     2509    +2     
=======================================
+ Hits        10386    10388    +2     
  Misses         60       60           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

<div data-testid="display-name" className="text-color-text-light">
{content.display_name}
{content.description && (
<div data-testid="description" className="line-clamp-2">
Copy link
Contributor Author

@acelaya acelaya Feb 13, 2025

Choose a reason for hiding this comment

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

I'm experimenting on cropping the description to up to 2 lines to avoid having an excessively big popover.

On the other hand, descriptions are 250-character long tops, so maybe we can simply let them be shown in full.

The problem with cropping is that perhaps we should then add the full content in the element's title attribute, but for that to work, we need #6821

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, descriptions are 250-character long tops, so maybe we can simply let them be shown in full.

Truncating at say 2 or 3 lines seems reasonable to me.

A max-length description looks like:

Full length description

@@ -161,7 +161,7 @@ export default function MarkdownView(props: MarkdownViewProps) {
open={!!popoverContent}
onClose={() => setPopoverContentAfterDelay(null)}
anchorElementRef={mentionsPopoverAnchorRef}
classes="px-3 py-2"
classes="px-3 py-2 !max-w-[75%]"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Limit the size of the popover to a maximum of 75% of the sidebar, to let it breathe.

@acelaya acelaya force-pushed the mention-popover-content branch from 6857ec3 to ff879d6 Compare February 13, 2025 14:26
@acelaya acelaya marked this pull request as ready for review February 14, 2025 10:18
@acelaya acelaya requested a review from robertknight February 14, 2025 10:18
src/sidebar/components/mentions/MentionPopoverContent.tsx Outdated Show resolved Hide resolved
src/types/api.ts Outdated Show resolved Hide resolved
@acelaya acelaya force-pushed the mention-popover-content branch from ff879d6 to efb214f Compare February 14, 2025 15:00
@acelaya acelaya requested a review from robertknight February 14, 2025 15:00
@acelaya acelaya force-pushed the mention-popover-content branch from efb214f to 3557f2d Compare February 14, 2025 15:04
@robertknight
Copy link
Member

We might want to set a minimum width or aspect ratio on the popover to avoid it looking too tall if the username and description are short:

Popover example

it('renders expected fields based on valid mention shape', () => {
const wrapper = createComponent(mention);
performAssertions(wrapper);
});
Copy link
Member

Choose a reason for hiding this comment

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

If each test here has different assertions, they could just be separate tests.

@@ -167,6 +170,10 @@ export type Mention = {
display_name: string | null;
/** Link to the user profile, if applicable */
link: string | null;
/** The user description/bio */
description: string | null;
/** The date in which the user joined, in ISO format */
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/** The date in which the user joined, in ISO format */
/** The date when the user joined, in ISO format */

<div data-testid="display-name" className="text-color-text-light">
{content.display_name}
{content.description && (
<div data-testid="description" className="line-clamp-2">
Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, descriptions are 250-character long tops, so maybe we can simply let them be shown in full.

Truncating at say 2 or 3 lines seems reasonable to me.

A max-length description looks like:

Full length description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement contents of the mention popover
2 participants