Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix left positioned tooltips being wrong and offset by fixed value #7551

Merged
merged 8 commits into from
Jan 19, 2022
Merged

Fix left positioned tooltips being wrong and offset by fixed value #7551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
33407c5
Fix tooltip left position being wrong and offset by fixed value
MadLittleMods Jan 15, 2022
c559e90
Remove debug logging
MadLittleMods Jan 15, 2022
6c6e246
Adjust spacing to be more similar to before
MadLittleMods Jan 15, 2022
deb1f30
Fix read receipt tooltip not positioning correctly
MadLittleMods Jan 15, 2022
511849b
Update test snapshot
MadLittleMods Jan 15, 2022
ffece09
Adjust vertical offset to be centered
MadLittleMods Jan 17, 2022
ee6f3f8
Beter vertical center
MadLittleMods Jan 17, 2022
dca8d2e
Merge branch 'develop' into madlittlemods/fix-tooltip-positions
MadLittleMods Jan 17, 2022
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
3 changes: 2 additions & 1 deletion res/css/views/elements/_Tooltip.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ limitations under the License.
font-weight: 500;
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can't remove the element-web reviewer? I don't want to assign for review until ready.

Seems to be locked 🤔

Copy link
Member

Choose a reason for hiding this comment

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

not sure why - was this opened as a draft from the start or converted to a draft?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was created normally. And I see the system note for converting to draft here:

Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe it's something to do with the code owner but I don't think I did anything to trigger that (edit: maybe not, seems to be the same on other PRs). Usually it assigns element-web but I can remove it.

max-width: 200px;
word-break: break-word;
margin-right: 50px;
margin-left: 6px;
margin-right: 6px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now using a consistent offset from both the left and right.


background-color: #21262C; // Same on both themes
color: $accent-fg-color;
Expand Down
4 changes: 4 additions & 0 deletions res/css/views/rooms/_EventTile.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ $left-gutter: 64px;
.mx_EventTile {
.mx_EventTile_receiptSent,
.mx_EventTile_receiptSending {
// Give it some dimensions so the tooltip can position properly
display: inline-block;
width: 14px;
height: 14px;
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In the before, it was just positioned by the a random ~50px offset from the right side which was a fixed size for all Alignment.Left tooltips regardless of the element size. Now it positions from the actual element.

Before After

Copy link
Member

Choose a reason for hiding this comment

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

The v-align is better here but still slightly off, now the tooltip looks a couple px too high

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can change the vertical alignment on this specific tooltip but this probably comes down to the vertical tooltip calculation code which I didn't touch in this PR and also has arbitrary offsets in it. Ideally that code should vertically center things just fine. We can address the vertical problems in another PR because I don't want to keep re-doing the before/after screenshots, etc

It's 20px above and below the read receipt now:

// We don't use `position: relative` on the element because then it won't line
// up with the other read receipts

Expand Down
4 changes: 2 additions & 2 deletions src/components/views/elements/Tooltip.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,8 +117,8 @@ export default class Tooltip extends React.Component<ITooltipProps> {
);
const baseTop = (parentBox.top - 2 + this.props.yOffset) + window.pageYOffset;
const top = baseTop + offset;
const right = width - parentBox.right - window.pageXOffset - 16;
const left = parentBox.right + window.pageXOffset + 6;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing arbitrary offsets - 16 and + 6

const right = width - parentBox.left - window.pageXOffset;
Copy link
Contributor Author

@MadLittleMods MadLittleMods Jan 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

AFAICT, previously this was a logic mistake. Although it worked well enough for all of the icons around the app.

We have the whole width of the viewport, then want to subtract the left position of the element in question to get the space to the right to position the tooltip at.

const left = parentBox.right + window.pageXOffset;
const horizontalCenter = (
parentBox.left - window.pageXOffset + (parentWidth / 2)
);
Expand Down
2 changes: 1 addition & 1 deletion src/components/views/rooms/EventTile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1728,7 +1728,7 @@ class SentReceipt extends React.PureComponent<ISentReceiptProps, ISentReceiptSta
}
// The yOffset is somewhat arbitrary - it just brings the tooltip down to be more associated
// with the read receipt.
tooltip = <Tooltip className="mx_EventTile_readAvatars_receiptTooltip" label={label} yOffset={20} />;
tooltip = <Tooltip className="mx_EventTile_readAvatars_receiptTooltip" label={label} yOffset={3} />;
}

return (
Expand Down
2 changes: 1 addition & 1 deletion test/components/views/elements/__snapshots__/TooltipTarget-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
exports[`<TooltipTarget /> displays tooltip on mouseover 1`] = `
<div
class="mx_Tooltip test tooltipClassName mx_Tooltip_visible"
style="right: 1008px; top: -26px; display: block;"
style="right: 1024px; top: -26px; display: block;"
>
<div
class="mx_Tooltip_chevron"
Expand Down