-
Notifications
You must be signed in to change notification settings - Fork 78
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
Conversation
There was a problem hiding this 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!
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. |
There was a problem hiding this 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!
apps/antalmanac/src/components/Calendar/toolbar/ScheduleSelect/ScheduleSelect.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/toolbar/ScheduleSelect/ScheduleSelect.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/toolbar/ScheduleSelect/ScheduleSelect.tsx
Outdated
Show resolved
Hide resolved
apps/antalmanac/src/components/Calendar/toolbar/ScheduleSelect/ScheduleSelect.tsx
Outdated
Show resolved
Hide resolved
I pushed new changes, let me know if these resolved the issues you had in mind! |
There was a problem hiding this 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!
Summary
Added an alt text pop up that displays the full names of schedules who's names were truncated.
Key features:
Test Plan
Issues
Closes #1072