-
Notifications
You must be signed in to change notification settings - Fork 17
fix: enable mobile-friendly tooltip behavior (Fixes #459) #476
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
base: main
Are you sure you want to change the base?
Conversation
|
@Roaring30s is attempting to deploy a commit to the Livepeer Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@Roaring30s thanks for your contribution! Just checked the preview. This is extremely valuable to the project. 🙏 Looks like some lining errors but after those are fixed @ECWireless can review and merge it in. |
|
@rickstaa Ill get those linting errors addressed. |
…into fix-tooltips Merge origin/fix-tooltips into fix-tooltips
|
@ECWireless linting error fixed. Should be ready for review. |
ECWireless
left a comment
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.
|
@ECWireless Good catch. Ill get this fixed today. Thanks for the review and feedback. |
|
@ECWireless I added your suggestion. I also tried on Chrome with an iPhone to see if I could get that exact same error and I didn't see it. I will share my screenshot below to what I saw clicking on the same tooltip. Here is the AI summary to aid in your review: Component Architecture RationaleSeparated Note: ExplorerTooltip was becoming too cluttered with the hook logic and event listeners needed to fix the mobile tooltip bug of it not disappearing when someone would scroll or do other touch events on mobile.
Implementation DetailsMobile Tooltip Improvements:
These changes ensure the mobile tooltip behaves correctly in various container contexts and provides a better user experience on touch devices. Finally, I didn't see the point making a custom hook for all the useEffect and state management work observed in the MobileTooltips since it will be used once and strictly applies to that small component but feel free to let me know your thoughts or any suggestions. The only benefit of adding said custom hook would be too improve testability.
|


What / Why
What changed:
ExplorerTooltipby conditionally renderingPopoveron touch devices instead ofTooltip(which only supports hover)Popover.Arrowto the mobile popover to match the desktop tooltip’s visual designWhy:
Issue
Fixes #459
Reproduction + Verification
Before:
After:
Verified:
Breaking / Risk / Rollout
No breaking changes. This is a backward-compatible enhancement.
Desktop:
Mobile:
Risk assessment:
ExplorerTooltipcomponentEdge cases handled:
openprop when providedonOpenChangecallback functionality