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

Fix layout and visual regressions around default avatars #10031

Merged
merged 6 commits into from
Jan 31, 2023
Merged

Fix layout and visual regressions around default avatars #10031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c461d9b
Fix regression in Space Panel default avatar colour
t3chguy Jan 31, 2023
62c6281
Fix regression with User Menu default avatar margins
t3chguy Jan 31, 2023
5a4b29b
Fix issue with default avatar read receipt positions being ignored
t3chguy Jan 31, 2023
ab8ce50
Add tests
t3chguy Jan 31, 2023
a2a5a15
Update snapshot
t3chguy Jan 31, 2023
cae28d1
Iterate
t3chguy Jan 31, 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
6 changes: 2 additions & 4 deletions cypress/e2e/spaces/spaces.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,10 +153,7 @@ describe("Spaces", () => {

openSpaceCreateMenu().within(() => {
cy.get(".mx_SpaceCreateMenuType_private").click();
cy.get('.mx_SpaceBasicSettings_avatarContainer input[type="file"]').selectFile(
"cypress/fixtures/riot.png",
{ force: true },
);
// We don't set an avatar here to get a Percy snapshot of the default avatar style for spaces
cy.get('input[label="Address"]').should("not.exist");
cy.get('textarea[label="Description"]').type("This is a personal space to mourn Riot.im...");
cy.get('input[label="Name"]').type("This is my Riot{enter}");
Expand All @@ -169,6 +166,7 @@ describe("Spaces", () => {

cy.contains(".mx_RoomList .mx_RoomTile", "Sample Room").should("exist");
cy.contains(".mx_SpaceHierarchy_list .mx_SpaceHierarchy_roomTile", "Sample Room").should("exist");
cy.get(".mx_LeftPanel_outerWrapper").percySnapshotElement("Left panel with default avatar space");
});

it("should allow user to invite another to a space", () => {
Expand Down
9 changes: 3 additions & 6 deletions res/css/structures/_SpacePanel.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,14 +277,11 @@ $activeBorderColor: $primary-content;
.mx_BaseAvatar:not(.mx_UserMenu_userAvatar_BaseAvatar) .mx_BaseAvatar_initial {
color: $secondary-content;
border-radius: 8px;
background-color: $panel-actions;
font-size: $font-15px !important; /* override inline style */
font-weight: $font-semi-bold;
line-height: $font-18px;

& + .mx_BaseAvatar_image {
visibility: hidden;
}
/* override inline styles which are part of the default avatar style as these uses a monochrome style */
Copy link
Member

Choose a reason for hiding this comment

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

it feels like we shouldn't be using inline styles, especially if they they have to be overridden in the css file. but I guess that's an existing situation which isn't getting materially worse here.

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 can't avoid using inline styles for things where the colours/sizes are defined in Javascript. The space panel is the one place which doesn't use coloured default avatars, we could override them in JS but this seems cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

Hrm, I guess. Does the font size also need to be defined in javascript?

(Also, I wonder if we should have a comment at the point that the styles are set in javascript that they will be overridden for the space panel)

Copy link
Member Author

Choose a reason for hiding this comment

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

not sure about the font size, that one is quite old

background-color: $panel-actions !important;
font-size: $font-15px !important;
}

.mx_SpaceTreeLevel {
Expand Down
2 changes: 2 additions & 0 deletions res/css/structures/_UserMenu.pcss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ limitations under the License.

.mx_UserMenu_userAvatar {
position: relative;
/* without this a default avatar will cause this to be 4px oversized and out of alignment */
display: inherit;

.mx_BaseAvatar {
pointer-events: none; /* makes the avatar non-draggable */
Expand Down
30 changes: 13 additions & 17 deletions src/components/views/avatars/BaseAvatar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import React, { useCallback, useContext, useEffect, useState } from "react";
import React, { CSSProperties, useCallback, useContext, useEffect, useState } from "react";
import classNames from "classnames";
import { ResizeMethod } from "matrix-js-sdk/src/@types/partials";
import { ClientEvent } from "matrix-js-sdk/src/client";
Expand Down Expand Up @@ -51,6 +51,7 @@ interface IProps {
inputRef?: React.RefObject<HTMLImageElement & HTMLSpanElement>;
className?: string;
tabIndex?: number;
style?: CSSProperties;
}

const calculateUrls = (url: string | undefined, urls: string[] | undefined, lowBandwidth: boolean): string[] => {
Expand Down Expand Up @@ -132,10 +133,17 @@ const BaseAvatar: React.FC<IProps> = (props) => {
onClick,
inputRef,
className,
style: parentStyle,
resizeMethod: _unused, // to keep it from being in `otherProps`
...otherProps
} = props;

const style = {
...parentStyle,
width: toPx(width),
height: toPx(height),
};

const [imageUrl, onError] = useImageUrl({ url, urls });

if (!imageUrl && defaultToInitialLetter && name) {
Expand All @@ -151,10 +159,7 @@ const BaseAvatar: React.FC<IProps> = (props) => {
className={classNames("mx_BaseAvatar", className)}
onClick={onClick}
inputRef={inputRef}
style={{
width: toPx(width),
height: toPx(height),
}}
style={style}
>
{avatar}
</AccessibleButton>
Expand All @@ -165,10 +170,7 @@ const BaseAvatar: React.FC<IProps> = (props) => {
className={classNames("mx_BaseAvatar", className)}
ref={inputRef}
{...otherProps}
style={{
width: toPx(width),
height: toPx(height),
}}
style={style}
role="presentation"
>
{avatar}
Expand All @@ -185,10 +187,7 @@ const BaseAvatar: React.FC<IProps> = (props) => {
src={imageUrl}
onClick={onClick}
onError={onError}
style={{
width: toPx(width),
height: toPx(height),
}}
style={style}
title={title}
alt={_t("Avatar")}
inputRef={inputRef}
Expand All @@ -202,10 +201,7 @@ const BaseAvatar: React.FC<IProps> = (props) => {
className={classNames("mx_BaseAvatar mx_BaseAvatar_image", className)}
src={imageUrl}
onError={onError}
style={{
width: toPx(width),
height: toPx(height),
}}
style={style}
title={title}
alt=""
ref={inputRef}
Expand Down
2 changes: 1 addition & 1 deletion test/components/views/avatars/__snapshots__/MemberAvatar-test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ exports[`MemberAvatar matches the snapshot 1`] = `
class="mx_BaseAvatar mx_BaseAvatar_image"
data-testid="avatar-img"
src="http://this.is.a.url//placekitten.com/400/400"
style="color: pink;"
style="color: pink; width: 35px; height: 35px;"
title="Hover title"
/>
</div>
Expand Down