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 all 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;
overflow: hidden;

&::after {
@include long-content-fade( $size: 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;
text-wrap: nowrap;

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

&__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