Fix imprecise tooltip positioning in PollListItem #31157
Conversation
github-actions
bot
added
the
Z-Community-PR
|
|
Optimize tooltip positioning to appear centered above poll question text while maintaining hover trigger across entire poll item area.
Mirzaian
force-pushed
the
fix/polls-tooltip-positioning-chat-settings
branch
from
96a37b1 to
0764a48
Compare
|
Not sure I quite understand the bug here - sounds like it's visual? In which case screenshots might be useful. Also,I'm not sure about the behaviour of opening the tooltip as soon as the cursor enters - normally there's a delay and this is handled automatically. |
|
Hey! Yes, its a visual UI issue. The tooltip was previously manually controlled and always appeared in a fixed position, which looked off for shorter poll titles. I have attached current/incoming screenshots to show the difference. The Tooltip now wraps the entire poll item, automatically centers above the title, and the whole item remains clickable. Regarding the hover delay, I have kept the default timing behavior. Incoming(current develop) behavior current(my changes) behavior |
|
It could likely also be fixed with css |
Good point, but I think that max-width approach could technically work as quick fix, but it would introduce responsiveness issues that make my current solution preferable. The decision is up to you guys. This just bothered me quite a bit, hence the PR. |
|
As sonarcloud says, the new code here will need a test. |
| expect(contentDiv).toBeTruthy(); | ||
| expect(questionSpan?.textContent).toBe("Question?"); |
There was a problem hiding this comment.
I'm not sure it's really relevant to this test to assert these things, or why we need the question div at all. We also really ought to be selecting things by role, or testId if that really isn't possible. In this case, this raises the question of why the inner div is necessary now that the tooltip is on something inside it: then we could just have the mouseenter/leave be on the li and select it using getByRole("listitem", { name: "01/01/70 Question?" }).
There was a problem hiding this comment.
Oki, I moved the mouse events to the li element and added the aria-label. I also added getByRole as you suggested.
| fireEvent.mouseLeave(contentDiv!); | ||
|
|
||
| // Component should still be functional after state changes | ||
| expect(container.querySelector(".mx_PollListItem")).toBeTruthy(); |
There was a problem hiding this comment.
I think the test really needs to assert that the tooltip actually appears.
There was a problem hiding this comment.
Oki, I replaced the querySelector approach with proper tooltip text assertion.
|
|
||
| // Test that mouse events work without error | ||
| fireEvent.mouseEnter(contentDiv!); | ||
| fireEvent.mouseLeave(contentDiv!); |
There was a problem hiding this comment.
I realise the rest of the test doesn't, so not a blocker, but new code probably ought to use user-event.
dbkr
left a comment
dbkr
left a comment
There was a problem hiding this comment.
Also needs another snapshot update I think
|
|
||
| // Verify tooltip content is present | ||
| const tooltip = getByText("View poll"); | ||
| expect(tooltip).toBeInTheDocument(); |
There was a problem hiding this comment.
Shouldn't this not be in the document at this point? Or at least not be visible? And shouldn't we be testing that's visible when hovering and then disappears again?
There was a problem hiding this comment.
I have updated it now and aligned it with the approach used in infotooltip-test.tsx
| await userEvent.hover(listItem); | ||
|
|
||
| // Wait for the tooltip to open | ||
| const questionSpan = getByText("Question?"); | ||
| const tooltip = await waitFor(() => { | ||
| const tooltip = document.getElementById(questionSpan.getAttribute("aria-labelledby")!); | ||
| expect(tooltip).toBeVisible(); | ||
| return tooltip; | ||
| }); | ||
| expect(tooltip).toHaveTextContent("View poll"); | ||
| }); |
There was a problem hiding this comment.
I just tested and, annoyingly, it looks like the test you copied from doesn't actually work, in that the assertion that the tooltip is visible passes even without hovering. I think this is because floating-ui hides the tooltip by positioning it offscreen or something. This means this is actually quite difficult to test. Writing a playwright test might be an option, or possibly mocking the tooltip component to remove that from the equation and just assert that it's been passed open=true. Possibly you could test manually for the tooltip position or classes, although this would be quite fragile. Sorry, this seems to be the classic case of pulling one tiny thread and a bunch of stuff unravelling.
4 participants
Fixes imprecise tooltip positioning in PollListItem
I found a bug. For shorter poll titles, the tooltip was not displayed optimally. This change fixes the imprecise tooltip positioning.
Checklist
public/exportedsymbols have accurate TSDoc documentation.