-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix tooltip alignment logic and removed hover blockage #4809 #4851
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: master
Are you sure you want to change the base?
Fix tooltip alignment logic and removed hover blockage #4809 #4851
Conversation
|
✅ All Jest tests passed! This PR is ready to merge. |
js/toolbar.js
Outdated
| console.log("Tooltip override block REACHED!"); | ||
| const icons = Array.from(document.querySelectorAll('.tooltipped')); | ||
| const right_icons = Array.from(document.querySelectorAll('ul.main.right a')); | ||
| const left_below_icons = Array.from(document.querySelectorAll('ul.aux.ledt a')); |
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.
ledt?
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.
she meant left probably
index.html
Outdated
| </li> | ||
| <li> | ||
| <a id="stop" class="left tooltipped"><i class="material-icons main">stop</i></a> | ||
| <a id="stop" class="left tooltipped"><i class="material-icons main ">stop</i></a> |
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.
why are you adding the space?
|
Can you be clear about what changes in the UX from this PR? |
|
✅ All Jest tests passed! This PR is ready to merge. |
|
✅ All Jest tests passed! This PR is ready to merge. |
Earlier tooltips used to have a fixed position. As a result:
With this modification:
In general, this results in a cleaner, easier-to-use, and less annoying user interface, particularly when interacting with closely spaced buttons. Also, Some changes in the diff are due to automatic code formatting applied by my IDE. |
|
I believe this is too much for something as simple as tooltips. |
Just to clarify the intent and scope of the change: the logic is not attaching tooltip listeners or recalculating positions on every hover event. The positioning is computed once after the toolbar is rendered, and tooltips are then initialized with a fixed position for each icon. There is no continuous work happening during hover itself, so this should not introduce runtime or performance overhead. The reason for handling this dynamically instead of hard-coding positions is maintainability and scalability. Today, tooltips overlap because their positions are static. If we keep the current approach, every time a new icon is added, removed, or reordered in the toolbar, tooltip positions would need to be manually adjusted again, otherwise the overlap issue will reappear. This becomes brittle over time. With the proposed approach:
So the goal here is not complexity for its own sake, but to avoid repeated manual fixes and ensure the tooltip system remains robust as the toolbar changes in the future. |
|
You talk about maintainability and scalability but your code is hardcoding the tool tip positions by accessing icons via indexes. I don't know how you make it dynamic if you're only giving different positions to the icons. How you have set up the tooltips is most definitely code smell and not scalable. |
According to the current positioning of the toolbar, if some icon will be added in the start of the right side icons, then they will be by default assigned tooltip position to be on the left , and if any of the icon is added in the below tooltip , it will be assigned as bottom , so that is what I am trying to do . |
|
Not really, you have things like |
This PR fixes incorrect tooltip placement by
Fixes #4809