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

Schedule Name Alt Text Expansion #1077

Merged
merged 4 commits into from
Dec 16, 2024
Merged

Conversation

Ardenjweed
Copy link
Contributor

Summary

Added an alt text pop up that displays the full names of schedules who's names were truncated.

Key features:

  • Tooltip only appears for truncated schedule names
  • Non-interactive tooltip that doesn't block clicking on other schedule items
  • 200ms delay to prevent unwanted popups during quick mouse movements
  • Maintains existing button styling and interaction behavior

AntalmanacHoverChange

Test Plan

  1. Create a schedule with a long name
  2. Verify the name is truncated in the dropdown menu
  3. Hover over the truncated name to confirm the tooltip appears showing the full name

Issues

Closes #1072

@KevinWu098 KevinWu098 self-requested a review December 15, 2024 06:28
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this PR! I have some reservations about the direction that we're taking here to resolve #1072.

As implemented, TooltipIfTruncated certainly does work, but I think that it may be overly complicated (and potentially confusing) to other maintainers. cloneElement is even described by the React docs as being "uncommon and can lead to fragile code."

I would prefer we choose a simpler path: wrap every schedule name in a tooltip, regardless of if the name is truncated

<Tooltip title={name} enterDelay={200} disableInteractive placement="right">

Though certainly more "heavy-handed", I feel that the codebase would be better served by this simpler approach (primarily in readability and approachability).

Let me know if you have any questions!

@Ardenjweed
Copy link
Contributor Author

I have made the changes as requested, let me know if you need anything to be changed.

As of right now the tooltip only shows if the user hovers over the text itself and not the whole button. Not sure if this is a change you would like to see or if how it works currently is perfect.

@KevinWu098 KevinWu098 self-requested a review December 15, 2024 20:37
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes! A few more comments on implementation, but otherwise it looks solid!

@Ardenjweed
Copy link
Contributor Author

I pushed new changes, let me know if these resolved the issues you had in mind!

@KevinWu098 KevinWu098 self-requested a review December 16, 2024 02:44
Copy link
Member

@KevinWu098 KevinWu098 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working on this!

@KevinWu098 KevinWu098 merged commit e454cee into icssc:main Dec 16, 2024
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.

Display schedule names on hover
2 participants