fix: popover right side arrow placement #17477
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
|
@mathieu-schnoor is attempting to deploy a commit to the primetek Team on Vercel. A member of the Team first needs to authorize it. |
mschn
force-pushed
the
fix-popover-arrow-right
branch
2 times, most recently
from
efcf680 to
e1b6290
Compare
| } | ||
| this.container?.style.setProperty('--overlayArrowLeft', `${arrowLeft}px`); |
There was a problem hiding this comment.
Isn't this line correct and should be reinstated? --overlayArrowLeft always is defined. if (containerOffset.left >= targetOffset.left) then the property is set to 0px. Your proposed change would have the property undefined. Maybe your change has no negative impact, have you tested your change for this case?
There was a problem hiding this comment.
it looks like --overlayArrowLeft is not used anymore in the code base, and that's the initial source of the bug.
With v18 and the introduction of the new theming system, --overlayArrowLeft was removed from the CSS file and we're using --p-popover-arrow-offset from the design tokens instead.
--p-popover-arrow-offset has a default value from the theme so if we don't override it here it will not be undefined
| @@ -330,13 +328,12 @@ export class Popover extends BaseComponent implements AfterContentInit, OnDestro | |||
|
|
|||
| const containerOffset = <any>getOffset(this.container); | |||
| const targetOffset = <any>getOffset(this.target); | |||
| const borderRadius = this.document.defaultView?.getComputedStyle(this.container!).getPropertyValue('border-radius'); | |||
| let arrowLeft = 0; | |||
| const targetWidth = getWidth(this.target); | |||
There was a problem hiding this comment.
For efficiency, move this line after line 333
There was a problem hiding this comment.
width calculation can be removed as you suggested
the arrow is not centered on the target element by default so there's no reason it should be in that case
|
|
||
| if (containerOffset.left < targetOffset.left) { | ||
| arrowLeft = targetOffset.left - containerOffset.left - parseFloat(borderRadius!) * 2; | ||
| const arrowLeft = targetOffset.left - containerOffset.left + targetWidth / 2; | ||
| this.container?.style.setProperty('--p-popover-arrow-offset', `${arrowLeft}px`); |
There was a problem hiding this comment.
Do you even need to center the arrow?
why not replace
arrowLeft = targetOffset.left - containerOffset.left + targetWidth / 2
with
arrowLeft = targetOffset.left - containerOffset.left
This should look good and targetWidth would not have to be calculated?
There was a problem hiding this comment.
centering the arrow is not consistent with the default behavior, you are right i will remove it.
if we simply use targetOffset.left - containerOffset.left it looks like this:
so it's still missing some offset here
I think the best is to reapply the --p-popover-arrow-offset value that comes from the theme,
that way it will be consistent with the default scenario:
mschn
force-pushed
the
fix-popover-arrow-right
branch
from
e1b6290 to
411a274
Compare
|
maybe I misunderstand, but what does 
changing the value of this property does not move the arrow which is the cause of the bug. |
|
Examining the v19 demo,
|
|
The Screenshot listed below shows the v17 working properly and the "inspected" page. As a result, I believe the correct fix is not to change the popover.ts file but to update the popoverstyle.ts (CSS file) similar to v17
|
Defect Fixes
#16406
Fix an issue with the popover arrow placement.
The issue can be reproduced on the showcase here when using a small viewport width:
https://primeng.org/popover#basic
Current issue
With this PR