-
Notifications
You must be signed in to change notification settings - Fork 2
fix: tooltip overlap in vertical math sidebar & restore missing icon #51
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
Conversation
…doteqdot' relation entry
…oltip component and useTooltip hook
src/components/Tooltip.js
Outdated
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.
Let's talk about the design of tooltip.
First, for a11y, please consider this example as your design spec.
<button aria-describedby="tooltip-1">Help</button>
<div id="tooltip-1" role="tooltip" hidden>Tooltip content here</div>
Second, do we want to have a tooltip "with" button, or a tooltip for "everything"? If we'd like to go for first, we should make an internal piece of our button component. In this case, the trigger component is Tab, so we could also consider have our own Tab with Tooltip support inside.
Third, we should use portal carefully, since portal component will be out of context of the trigger component, which might lead more issue potentially. Could we design the interface like
<Tooltip>
<Tab />
</Tooltip>
This could also avoid accessing trigger component by event instance, which is not straightforward to me.
Then, I think the current interface exposes too much. For example, we should only have position as enum like Position:: LEFT | RIGHT | TOP | BUTTOM instead of an actual number. The useTooltip should be private module for Tooltip, instead of a public one, no reason to share this hook for others after all. And, please always consider to leverage children for semantic usage like text in our case.
We can discuss this in our bi-weekly sync if you'd like.
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.
@WendellLiu Thank you for the suggestion! I’ve completed the refactor. Please let me know if anything else needs to be updated.
- Wrapped elements in
EditIconsTabwith Tooltip for better integration - Replaced
ReactDOM.createPortalwith direct positioning in Tooltip - Simplified tooltip logic using
cloneElementfor event handling - Removed the
useTooltiphook and centralized the tooltip logic - Added
aria-describedbyandrole="tooltip"for a11y compliance - Tooltip now uses enum-based position values (e.g., top, right, etc.)
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.
- Since it's a shared component, could we port it to
components/core/? - Do we actually need to calculate the left and top? Could we implement with absolute-relative positioning system with relative left and top like 0%, 100%, ...etc. If we are able to do so, we don't need ref from children anymore.
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.
@WendellLiu Thank you for the suggestion!
- Sure, updated!
- I agree that calculating the distance is not the most ideal approach. However, due to the "overflow-x / overflow-y problem" (reference link), I believe it’s not possible for
mathTabListto have both a vertical scrollbar and a tooltip that appears outside the section (without usingposition: fixed) at the same time.
If you are using visible for either overflow-x or overflow-y and something other than visible for the other, the visible value is interpreted as auto.(from reference link)
If we’d like to keep the tooltip behavior straightforward (using absolute-relative positioning), we might need to consider redesigning the mathTabList UI. Perhaps there’s an alternative way to present the math syntax tab outside the current area, which would help avoid the overflow-x issue. Please let me know if you have any suggestions or thoughts. Thanks a lot!
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.
I understand. The issue is that we have some sub-layouts, but we want to show tooltips over them. I'm not fully sure about the root cause, but it seems complex enough to require an ad-hoc solution.
After switching the math formula sidebar from horizontal to vertical, the tooltip overlaps with other elements(Screenshot 1). Since there's an "overflow-x / overflow-y problem" (reference link), simply setting
overflow-x: visibledidn’t fix it. So, I built a new tooltip component that avoids this overflow issue and displays correctly.Before:

After:

An icon was accidentally commented out. This PR restores the missing icon(
src/lib/tabs/math/relation.js).