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

Fix avatar upload prompt/tooltip floating wrong and permissions #5526

Merged
merged 4 commits into from
Jan 11, 2021

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Dec 27, 2020

Fixes element-hq/element-web#15832
Fixes element-hq/element-web#16016
Fixes element-hq/element-web#16103

Visually identical, but without the bugs.

@t3chguy t3chguy requested a review from a team December 27, 2020 21:14
Comment on lines +91 to +94
<div className={classNames("mx_Tooltip", {
"mx_Tooltip_visible": visible,
"mx_Tooltip_invisible": !visible,
})}>
Copy link
Member

Choose a reason for hiding this comment

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

it's a bit of a smell if we're ripping classes out of a component just to use them here. Can we make it a render mode of the Tooltip to do the work for us?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but it'd make it an even more confusing component and we need to rebuild out Tooltips altogether as it is

Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit torn because this is the sort of thing that we should probably avoid making worse, but the situation is so bad that it's on a shortlist of options.

@jryans might be best to review this

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issues are quite annoying visually, so I think it's better to accept this change and then revisit tooltips holistically later.

@turt2live turt2live requested a review from jryans December 29, 2020 16:34
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

A bit of an unusual approach, but I think it's okay to take on in this case.

Comment on lines +91 to +94
<div className={classNames("mx_Tooltip", {
"mx_Tooltip_visible": visible,
"mx_Tooltip_invisible": !visible,
})}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The issues are quite annoying visually, so I think it's better to accept this change and then revisit tooltips holistically later.

@t3chguy t3chguy merged commit 1c3adbc into develop Jan 11, 2021
@t3chguy t3chguy deleted the t3chguy/fix/15832 branch January 11, 2021 16:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants