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.

Sign up for GitHub

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

Jump to bottom

Fix Email tooltip of the user avatar in the chat report page is incorrectly positioned #16370

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Fix Email tooltip of the user avatar in the chat report page is incorrectly positioned #16370

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b4ade6e
determine if there is an overlapping element at top
hoangzinh Mar 21, 2023
aef1196
ensure it won't raise error on unsupported browser
hoangzinh Mar 21, 2023
107c607
Ensure it won't raise error if element not found
hoangzinh Mar 21, 2023
6ba71ec
update comment
hoangzinh Mar 22, 2023
eac6c73
Update function description from code review
hoangzinh Mar 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 30 additions & 2 deletions src/styles/getTooltipStyles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,33 @@ function computeHorizontalShift(windowWidth, xOffset, componentWidth, tooltipWid
return 0;
}

/**
* Determines if there is an overlapping element at the top of a given coordinate.
hoangzinh marked this conversation as resolved.
Show resolved Hide resolved
*
* @param {Number} xOffset - The distance between the left edge of the window
* and the left edge of the wrapped component.
* @param {Number} yOffset - The distance between the top edge of the window
* and the top edge of the wrapped component.
* @returns {Boolean}
*/
function isOverlappingAtTop(xOffset, yOffset) {
if (typeof document.elementFromPoint !== 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

ensure it won't raise error on unsupported browser

@hoangzinh which browser doesn't support this?

elementFromPoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my bad without check this one. 99.1% supported browser is a high number. According to screenshot above, some very old browsers don't support it, but small percent.

btw, do you mean from this comment is we need to check document.elementFromPoint is exist before use?

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually meant this check which you already did:

    const element = document.elementFromPoint(xOffset, yOffset);

    if (!element) {
        return false;
    }

Not elementFromPoint function itself

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, let's leave as it is now

return false;
}

const element = document.elementFromPoint(xOffset, yOffset);

Copy link
Contributor

Choose a reason for hiding this comment

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

Nab: uneccessary blank space

if (!element) {
return false;
}

const rect = element.getBoundingClientRect();

// Ensure it's not itself + overlapping with another element by checking if the yOffset is greater than the top of the element
// and less than the bottom of the element
return yOffset > rect.top && yOffset < rect.bottom;
}

/**
* Generate styles for the tooltip component.
*
Expand Down Expand Up @@ -86,9 +113,10 @@ export default function getTooltipStyles(
manualShiftVertical = 0,
) {
// Determine if the tooltip should display below the wrapped component.
// If a tooltip will try to render within GUTTER_WIDTH logical pixels of the top of the screen,
// If either a tooltip will try to render within GUTTER_WIDTH logical pixels of the top of the screen,
// Or the wrapped component is overlapping at top-left with another element
// we'll display it beneath its wrapped component rather than above it as usual.
const shouldShowBelow = (yOffset - tooltipHeight) < GUTTER_WIDTH;
const shouldShowBelow = (yOffset - tooltipHeight) < GUTTER_WIDTH || isOverlappingAtTop(xOffset, yOffset);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line introduced an edge case regression in #17844 , more context here #17844 (comment)


// Determine if we need to shift the tooltip horizontally to prevent it
// from displaying too near to the edge of the screen.
Expand Down