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

Reader: Make user profile header responsive #98184

Merged
merged 11 commits into from
Jan 15, 2025
Merged

Reader: Make user profile header responsive #98184

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3b92259
Set max-width to initial
DustyReagan Jan 9, 2025
5ff3417
Minor adjustment to gutters
DustyReagan Jan 9, 2025
f401fc5
Readd header center alignment
DustyReagan Jan 9, 2025
49bfc25
Merge branch 'trunk' into fix/reader-user-header-img
DustyReagan Jan 10, 2025
14f2339
Responsive header mostly working
DustyReagan Jan 10, 2025
c8a96fa
Minor tweaks
DustyReagan Jan 10, 2025
ec10617
Properly type UserProfileHeader
DustyReagan Jan 10, 2025
02e7f1e
Use CSS media queries for user profile header avatar
DustyReagan Jan 13, 2025
8596b12
Fix long-content-fade
DustyReagan Jan 14, 2025
a14b8fb
Add nowrap to display-name
DustyReagan Jan 14, 2025
d5eb852
Add overflow hidden
DustyReagan Jan 14, 2025
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
16 changes: 11 additions & 5 deletions client/reader/user-stream/components/user-profile-header/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import page from '@automattic/calypso-router';
import { useTranslate } from 'i18n-calypso';
import ReaderAuthorLink from 'calypso/blocks/reader-author-link';
import ReaderAvatar from 'calypso/blocks/reader-avatar';
import SectionNav from 'calypso/components/section-nav';
import NavItem from 'calypso/components/section-nav/item';
Expand Down Expand Up @@ -33,15 +32,22 @@ const UserProfileHeader = ( { user }: UserProfileHeaderProps ): JSX.Element => {

const selectedTab = navigationItems.find( ( item ) => item.selected )?.label || '';

const avatarElement = (
<ReaderAvatar author={ { ...user, has_avatar: !! user.avatar_URL } } iconSize={ 116 } />
);

return (
<div className="user-profile-header">
<header className="user-profile-header__main">
<ReaderAvatar author={ { ...user, has_avatar: !! user.avatar_URL } } />
<div className="user-profile-header__avatar user-profile-header__avatar-desktop">
{ avatarElement }
</div>
<div className="user-profile-header__details">
<div className="user-profile-header__display-name">
<ReaderAuthorLink author={ { name: user.display_name } }>
{ user.display_name }
</ReaderAuthorLink>
<div className="user-profile-header__avatar user-profile-header__avatar-mobile">
{ avatarElement }
</div>
{ user.display_name }
</div>
{ user.bio && (
<div className="user-profile-header__bio">
Expand Down
80 changes: 72 additions & 8 deletions client/reader/user-stream/components/user-profile-header/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,95 @@
@import '@wordpress/base-styles/mixins';
@import '@wordpress/base-styles/breakpoints';

.is-section-reader {
.user-profile-header {
max-width: 768px;
margin: 0 auto;
padding: 0 16px;

&__main {
align-items: center;
display: flex;
padding: 36px 0 24px;
align-items: center;

.reader-avatar {
margin: 0 24px 0 0;
@include breakpoint-deprecated( '<660px' ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we use @include break-small or @include break-medium here instead of using the deprecated mixin?

Copy link
Member Author

@DustyReagan DustyReagan Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ideally, yes. This breakpoint combats additional margin added here. There is no standard breakpoint for 660px, and rather than pull on the string to unravel the use of 660px here, I opted to use breakpoint-deprecated( '<660px' ).

Copy link
Member

Choose a reason for hiding this comment

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

If its possible to change both and not use deprecated one then maybe that's best. Otherwise this is fine too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to land this PR using breakpoint-deprecated( '<660px' ). I'll timebox an investigation to see if we can refactor the deprecated breakpoint here, and open a separate PR to replace if it seems feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noting that this style would be better at max-width: 781px for the Lists component, as there's no breakpoint-deprecated( '<660px' ) on that page that's when the sidebar collapses.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noting that this style would be better at max-width: 781px for the Lists component, as there's no breakpoint-deprecated( '<660px' ) on that page that's when the sidebar collapses.

*We can handle this as part of a separate PR cleaning up styles for the Lists component.

padding-top: 20px;
}
}

&__avatar {
margin-right: 24px;
flex-shrink: 0;
width: 116px;
height: 116px;

img {
border-radius: 100%;
width: 100%;
height: 100%;
}

&-mobile {
width: 72px;
height: 72px;
margin-right: 16px;
display: block;

@include break-mobile {
display: none;
}
}

&-desktop {
display: none;

@include break-mobile {
display: block;
margin-right: 24px;
width: 116px;
height: 116px;
}
}
}

&__details {
flex: 1;
min-width: 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the min-width value here? I've checked and it doesn't seem necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's helpful for really long text. For example, a long name will prevent the flexbox from shrinking past a certain width due to it sizing to the content, and then the bio will also run off the screen with the name. This prevents that.

}

&__display-name {
font-family: Recoleta, sans-serif;
font-size: 2rem;
font-family: 'Noto Serif', Georgia, 'Times New Roman', Times, serif;
font-size: $font-title-medium;
font-weight: 600;
line-height: 1.5;
margin-bottom: 16px;
display: flex;
align-items: center;

@include break-mobile {
font-size: $font-title-large;
margin-bottom: 8px;
display: block;
}

&::after {
@include long-content-fade( $size: 15% );
height: 16px * 1.7;
top: auto;
Copy link
Contributor

@artemiomorales artemiomorales Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the long-content-fade isn't being properly applied. It has position: absolute, so the __display-name needs position: relative so that the fade doesn't appear at the bottom of the page. Even after adding the relative positioning, I don't think it's quite working since the display name content wraps:

Screenshot 2025-01-13 at 19 27 46

I believe we're looking more for something along the following lines, right? To do that, we'd need to add text-wrap: nowrap to the __display-name, and probably fix the positioning of the pseudoelement, as it doesn't seem to be elegantly covering the overflowing letters:

Screenshot 2025-01-13 at 19 32 26

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good find! I updated it, so now it's fading whatever flows out into the 16px padding of the header, and added nowrap to the name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok great! Looking much better at smaller resolutions.

What's the expected behavior when we have long names at higher resolutions? It still looks off in those cases. Should we put overflow: hidden to hide the name past the pseudoelement, or should we wrap the text?

Screenshot 2025-01-14 at 10 24 51

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops! I added overflow: hidden. Thanks!

or should we wrap the text?

I'm a bit torn on this. /read/feeds/{feedId} wraps the text, but I'm not sure I like the result. 😅 I'm leaning toward hiding the overflow because it keeps the layout behavior predictable, especially on small screens. We could try this on for size and follow up if it's not working for our users? I could also be convinced otherwise if you feel strongly about it. 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

@artemiomorales, I'm going to go ahead and ship this PR since it's behind the flag. I'm happy to follow up on it in a new PR! ☺️

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to go ahead and ship this PR since it's behind the flag. I'm happy to follow up on it in a new PR! ☺️

Ok! The overflow sounds good to me. We can also iterate if need be :D

}
}

&__bio {
/* stylelint-disable-next-line scales/font-sizes */
font-size: 1.125rem;
font-size: $font-body-large;
color: var( --color-text-subtle );

@include break-mobile {
font-size: $font-body;
}
}

&__bio-desc {
margin-bottom: 0;
margin: 0;
}

.section-nav {
Expand Down
Loading