-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Async-loaded Components (~18 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Thanks for the review, @davemart-in!
It's tricky because the However, I lifted the styling from Screen.Recording.2025-01-10.at.2.52.39.PM.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the header responsive.
client/reader/user-stream/components/user-profile-header/index.tsx
Outdated
Show resolved
Hide resolved
|
||
.reader-avatar { | ||
margin: 0 24px 0 0; | ||
@include breakpoint-deprecated( '<660px' ) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
client/reader/user-stream/components/user-profile-header/style.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing in responsive Chrome on desktop, the styles look pretty good to me! It collapses nicely. A couple of comments below.
|
||
&__details { | ||
flex: 1; | ||
min-width: 0; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
&::after { | ||
@include long-content-fade( $size: 15% ); | ||
height: 16px * 1.7; | ||
top: auto; |
There was a problem hiding this comment.
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:
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:
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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. 😆
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
Related to https://github.com/Automattic/loop/issues/296
Proposed Changes
ReaderFeedHeader
. However, it does not implementReaderFeedHeader
becauseReaderFeedHeader
takes a site and feed as props and contains site-centric functionality.Why are these changes being made?
Testing Instructions
Pre-merge Checklist