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

Reader: add placeholder for reader-related-card-v2 #8334

Merged
merged 4 commits into from
Sep 29, 2016

Conversation

bluefuton
Copy link
Contributor

@bluefuton bluefuton commented Sep 29, 2016

Add a placeholder when related posts blocks are loading in the new Reader full post view.

This will help us when we need to scroll to a specific spot on the page before everything's loaded (e.g. for comments anchor #7892).

Outstanding tasks:

  • Turn off long content fade (cc @jancavan)
  • Uncomment logic to only use placeholder when post is missing or still in pending state

@bluefuton bluefuton added [Status] In Progress [Feature] Reader The reader site on Calypso. labels Sep 29, 2016
@bluefuton bluefuton added this to the Reader: Full Post Refresh Cleanup milestone Sep 29, 2016
@bluefuton bluefuton self-assigned this Sep 29, 2016
@matticbot
Copy link
Contributor

@@ -60,6 +91,10 @@ export function RelatedPostCard( { post, site, onPostClick = noop, onSiteClick =
'has-thumbnail': !! featuredImage
} );

//if ( ! post || post.state == 'pending' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For testing only - see todo item above :)

Uncomment logic to only use placeholder when post is missing or still in pending state

Uncommented now!

Copy link
Contributor

Choose a reason for hiding this comment

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

:face-palm:

@bluefuton bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Sep 29, 2016
Copy link
Contributor

@samouri samouri left a comment

Choose a reason for hiding this comment

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

i have much to learn

@@ -442,3 +442,40 @@ $reader-related-card-v2-breakpoint-small: "( max-width: 535px )";
max-height: none;
}
}

// Placeholders
.reader-related-card-v2.is-placeholder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to nest the .is-modifier instead of the selectors? I'll update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jancavan!

Copy link
Contributor

Choose a reason for hiding this comment

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

@bluefuton Actually nvm. It doesn't make sense in this case. I tried and we're going to end up with too many .is-modifers.

@jancavan jancavan added [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] In Progress labels Sep 29, 2016
@jancavan
Copy link
Contributor

jancavan commented Sep 29, 2016

The stacked RPs have placeholder images too wide:

screenshot 2016-09-29 13 16 07

I can look into this @bluefuton.

@jancavan jancavan added [Status] In Progress and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 29, 2016
@jancavan jancavan added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Sep 29, 2016
@jancavan
Copy link
Contributor

@bluefuton Fixed in: 8301f12

Before:
screenshot 2016-09-29 13 16 07

After:
screenshot 2016-09-29 14 44 01

@bluefuton
Copy link
Contributor Author

Thank you @jancavan!

@jancavan
Copy link
Contributor

LGTM 🚢

@jancavan jancavan added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 29, 2016
@bluefuton bluefuton merged commit 285b5bb into master Sep 29, 2016
@bluefuton bluefuton deleted the add/reader/related-posts-placeholders branch September 29, 2016 22:37
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants