-
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 full post page - add global sidebar and content frame #91990
base: trunk
Are you sure you want to change the base?
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: Sections (~252 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~27 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. |
// add extra div wrapper for consistent content frame layout/styling for reader. | ||
<div> |
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.
While the changes here look like a lot, all I have done is wrap everything in an empty div. This makes the structure more consistent with the rest of the reader and able to inherit the content frame styles appropriately.
recordAction( 'full_post_close' ); | ||
recordGaEvent( 'Closed Full Post Dialog' ); | ||
recordTrackForPost( 'calypso_reader_article_closed', this.props.post ); |
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.
Moved these into the controller where the onClose is defined so closing via the sidebar can send them as well.
display: grid; | ||
grid-template-columns: auto 1fr 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.
The related card width was expanding with extra long site names, breaking layout and creating hidden overflow for the full post content. This helps ensure the proper width for the site title region of the meta.
if ( props.onClose ) { | ||
ev.preventDefault(); | ||
ev.stopPropagation(); | ||
props.onClose(); |
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.
Having an onClose allows us to use the callback using the page.back
handler, so that the back button always goes back to the last place and updates browser history accordingly. Just using context.lastRoute as the href creates an infinite loop of between the last 2 items when using the back button. Also, updating the sidebar to always use page.back history would be problematic for other reasons - this is only necessary from the full post page currently.
On top of the callback using page.back, it also allows us to fire the corresponding tracks/stats events when closing the full post page.
@@ -61,6 +91,21 @@ export function feedPost( context, next ) { | |||
referralStream={ context.lastRoute } | |||
/> | |||
); | |||
|
|||
if ( isUserLoggedIn( state ) ) { | |||
context.secondary = ( |
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.
Note we add the sidebar here in the full post controller instead of using the sidebar
middleware function we use in the main reader. This is because the full post page needs special handling/props for back button history and tracking events passed into the sidebar.
returnPath={ lastRoute } | ||
onClose={ closer } |
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.
While we are already handling the return path and back functionality with the onClose, supplying the returnPath allows the breadcrumb url for the anchor link to be accurate.
Looking sharp! I noticed two small things:
CleanShot.2024-06-21.at.15.15.35.mp4I prefer the padding at the smaller resolution (a bit more padding) than the one that feels a tad too tight.
CleanShot.2024-06-21.at.15.18.04.mp4Once those changes are in, this should be ready to ship! |
I added this on purpose. If a user is browsing a stream on mobile, they shouldnt have to toggle open the sidebar every time they go back to the stream from full post. Avoiding "More clicks, less engagement" 😅 |
Gotcha. In that case, let me spend a little bit of time polishing the style up if you don't mind. |
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.
Sounds good @davemart-in ! I updated the gutter padding and left a couple notes below.
display: none; | ||
position: absolute; | ||
top: 0; | ||
left: 0; | ||
right: unset; |
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.
re: adjusting the back button positioning this is where that is currently set (relative absolute on the content frame)
@@ -140,15 +107,13 @@ | |||
|
|||
.reader-full-post__story { | |||
max-width: 720px; | |||
padding: 0 24px; |
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.
Re: gutter padding. I got rid of the conflicting sources of padding introduced on different elements at different breakpoints and it should be consolidated here. Feel free to adjust it, but we also need to adjust the breakpoint to hide the sidebar along with this.
|
||
@include breakpoint-deprecated( "<660px" ) { | ||
// hide the sidebar when it cannot fit to the side of the content. | ||
@media (max-width: 1298px) { |
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.
Re: gutter padding. If we adjust that we will need to tweak this value similarly. I may look into using variables to connect these values better.
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 |
Related to # #92025
Proposed Changes
BEFORE
![Screenshot 2024-06-21 at 1 43 14 PM](https://private-user-images.githubusercontent.com/28742426/341847330-d184dcd8-3b3e-4b20-886d-42e416a24cbf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkzOTY5MDAsIm5iZiI6MTcxOTM5NjYwMCwicGF0aCI6Ii8yODc0MjQyNi8zNDE4NDczMzAtZDE4NGRjZDgtM2IzZS00YjIwLTg4NmQtNDJlNDE2YTI0Y2JmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjI2VDEwMTAwMFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTFhNTNmZjNiMTRkYjExODcxOTdkOTk4ODVkMDgzMGQyOGFjMTgzOGJmNmRjM2EwZTdkZTdkNzU2NGI3OWQzZDUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.rlCxSG3OSGP9lZo7AaIBuEd8p41jWbUjenOFfIW6D8I)
AFTER
![Screenshot 2024-06-21 at 1 42 53 PM](https://private-user-images.githubusercontent.com/28742426/341847319-f5b20aac-299a-4307-8e61-2c8a13a60bdf.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTkzOTY5MDAsIm5iZiI6MTcxOTM5NjYwMCwicGF0aCI6Ii8yODc0MjQyNi8zNDE4NDczMTktZjViMjBhYWMtMjk5YS00MzA3LThlNjEtMmM4YTEzYTYwYmRmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MjYlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjI2VDEwMTAwMFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTdkZjMwMzc3OThlYmYzNTVhOTJkZmNlM2UzYjI4MjAwMjViZWVkN2M4MzlhZTA1NjZjMDliOGVkNzNjM2JmMzUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.pmhPAS5HHWZHdGB0zkPko8sknaZE_KP-uSS0h0KaqcQ)
Why are these changes being made?
Testing Instructions
article-closed
tracking event with populated post data. you can use tracks-vigilante to help verify.Pre-merge Checklist