Skip to content
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

#7071 enhance edge detection for tooltips #7072

Merged
merged 2 commits into from
Jan 13, 2025

Conversation

iessa-pragg-ctct
Copy link
Contributor

@iessa-pragg-ctct iessa-pragg-ctct commented Jan 10, 2025

###Defect Fixes
#7071

Adding some edge detection when position is top or bottom and the tooltip is close to the left/right edge of the screen.

As a consequence of this when the tooltip is adjusted to fit inside the screen the tooltip arrow ends up being in the wrong position, fixed that as well.

Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Updated (UTC)
primevue ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 7:15am
primevue-v3 ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 7:15am

Comment on lines +430 to +433
if (hostOffset.left + tooltipWidth > viewportWidth) {
// accounting for a scrollbar being present, getViewport() width includes scrollbars
left = Math.floor(hostOffset.left + elementWidth - tooltipWidth);
}
Copy link
Contributor Author

@iessa-pragg-ctct iessa-pragg-ctct Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to viewport width - tooltip width, but the current viewport helper returns the viewport width including the scrollbar.

This works well enough though as a workaround.

Using floor as offset values are floating points which causes off by 1px havoc, floor here as we are calculating a left offset, to be on screen a smaller number is needed.

window (1400px)
left (1400.359275px) -- up to the browser vendor whether to round up or down to the nearest pixel

Comment on lines 439 to 444
let elementRelativeCenter = Math.abs(this.getHostOffset(tooltipElement).left - hostOffset.left) + elementWidth / 2;

arrowElement.style.top = null;
arrowElement.style.right = null;
arrowElement.style.bottom = '0';
arrowElement.style.left = elementRelativeCenter + 'px';
Copy link
Contributor Author

@iessa-pragg-ctct iessa-pragg-ctct Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to move the arrow positioning out of preAlign as we need to access the final position of the tooltip in order to position the arrow properly.

As a bonus this removes the somewhat hard to grok ternary that was setting this before.

@tugcekucukoglu
Copy link
Member

Screenshot 2025-01-10 at 09 40 37

There are calculation problems on top and bottom.

@iessa-pragg-ctct
Copy link
Contributor Author

Will have a poke at them, thanks for testing.

@iessa-pragg-ctct
Copy link
Contributor Author

iessa-pragg-ctct commented Jan 10, 2025

Apparently my math is terrible, had things backwards.

I believe it should be right now, also made the tooltip only check half the width to see if it has enough space to fit on the left side of the screen, this was left aligning the tooltip before it needed to.

There is a 1px error going on with the arrow at some sizes/positions, however I'm fairly sure that was pre existing, I see it on the current official docs as well, it can be seen in the first two screenshots here.
image
image
image
image
image
image

@tugcekucukoglu tugcekucukoglu merged commit b47856b into primefaces:master Jan 13, 2025
2 checks passed
@tugcekucukoglu
Copy link
Member

Thank you so much for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants