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.

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

Move reader-main to reader/components #28272

Merged
merged 2 commits into from
Nov 27, 2018
Merged

Move reader-main to reader/components #28272

merged 2 commits into from
Nov 27, 2018

Conversation

bluefuton
Copy link
Contributor

Changes proposed in this Pull Request

A number of Reader-specific components and blocks currently live in the main components and blocks directories, even though they are not used elsewhere in Calypso. I feel that these components and blocks should live inside client/reader if they are not likely to be reused outside Reader. See also: #28161.

What's reader-main anyway?

This class is used by pieces of the Reader to indicate they want "editorial" styles. Notably, this overrides the background color of the document and is used as a hook by other parts to override styles.

Testing instructions

Load the Reader at http://calypso.localhost:3000 and ensure that all Reader streams have a white background.

@bluefuton bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. [Type] Janitorial labels Nov 5, 2018
@bluefuton bluefuton self-assigned this Nov 5, 2018
@bluefuton bluefuton requested a review from a team November 5, 2018 03:07
@matticbot
Copy link
Contributor

@@ -113,6 +113,10 @@ export class FullPostView extends React.Component {
this.attemptToSendPageView();
}

if ( this.props.shouldShowComments && ! prevProps.shouldShowComments ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an unrelated change - the linter picked up on usage of componentWillReceiveProps in Reader full post.

To test, make sure that a full post URL with #comments suffix still takes you to the comments section of a full post, e.g.:

http://calypso.localhost:3000/read/blogs/140522955/posts/2773#comments

Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion for the future: Lazy loading images can push the scroll of the page further down, moving the comments section off-screen. I'd imagine this could be solved if we had the height information of the images.

Copy link
Contributor

@jsnmoon jsnmoon left a comment

Choose a reason for hiding this comment

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

Code changes look good and reader seems to work as expected. Needs a rebase to proceed, though.

assets/stylesheets/_components.scss Show resolved Hide resolved
@@ -113,6 +113,10 @@ export class FullPostView extends React.Component {
this.attemptToSendPageView();
}

if ( this.props.shouldShowComments && ! prevProps.shouldShowComments ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion for the future: Lazy loading images can push the scroll of the page further down, moving the comments section off-screen. I'd imagine this could be solved if we had the height information of the images.

@bluefuton bluefuton merged commit b96b2cc into master Nov 27, 2018
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 27, 2018
@bluefuton bluefuton deleted the move/reader-main branch November 27, 2018 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants