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

Jump to bottom

Reader Stream Refresh: add photo-only card #8843

Merged
merged 8 commits into from
Oct 19, 2016
Merged

Reader Stream Refresh: add photo-only card #8843

merged 8 commits into from
Oct 19, 2016

Conversation

jancavan
Copy link
Contributor

@jancavan jancavan commented Oct 18, 2016 •
edited by bluefuton
Loading

Fixes #8518

1af553b4-8bc4-11e6-8329-e839005b2279

@jancavan jancavan added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [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. labels Oct 18, 2016
@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/reader/photo-only-card-refresh-stream

This was referenced Oct 18, 2016
Closed
Closed
@bluefuton bluefuton added this to the Reader: Stream Refresh milestone Oct 19, 2016
@bluefuton
Copy link
Contributor

This is looking awesome @jancavan! A few selected screenshots:

screen shot 2016-10-19 at 15 32 49

screen shot 2016-10-19 at 15 32 59

screen shot 2016-10-19 at 15 33 16

cc @fraying

@bluefuton
Copy link
Contributor

One minor issue is that I sometimes see a flash of the standard card whilst the photo-only card is loading. It's because the card hasn't received the display_type in time for the first render. Perhaps we can do that earlier @blowery?

Video: https://cloudup.com/cldHLUdue13

@bluefuton bluefuton changed the title Reader: Add photo-only card Reader Stream Refresh: add photo-only card Oct 19, 2016
@fraying
Copy link
Contributor

fraying commented Oct 19, 2016

We're going to be hiding the excerpts, right?

@jancavan
Copy link
Contributor Author

@bluefuton As mentioned here: #8823 (comment)

If the post has 1 or more images and less than 130 characters: Photo card (using featured image or first image).

@bluefuton
Copy link
Contributor

bluefuton commented Oct 19, 2016 •
edited
Loading

We're going to be hiding the excerpts, right?

Ah yes, we are indeed :) As of b995d5c the excerpt is hidden and the cards look like this:

screen shot 2016-10-19 at 20 36 31

@blowery
Copy link
Contributor

blowery commented Oct 19, 2016

Changed up the background to use a linear gradient instead of the full overlay, which makes many cards look a fair bit better. No more muted colors! :)

Also expanded the title. It was getting chopped at 50 characters, which felt way too short. Instead, dynamically truncate it using overflow and text-overflow along with an ellipsis on supported browsers.

@jancavan
Copy link
Contributor Author

Thanks @blowery 👏

In a3ad975, made a minor adjustment to the linear gradient overlay so it doesn't end abruptly (only obvious in lighter backgrounds).

Before:
screenshot 2016-10-19 09 56 11

After:
screenshot 2016-10-19 09 56 53

@blowery
Copy link
Contributor

blowery commented Oct 19, 2016

@jancavan woo, that's looking nice!

blowery
blowery reviewed Oct 19, 2016
View reviewed changes
client/blocks/reader-post-card/index.jsx
@@ -96,7 +96,7 @@ export default class RefreshPostCard extends React.Component {
const featuredImage = post.canonical_image;
const isPhotoOnly = post.display_type & DisplayTypes.PHOTO_ONLY;
const title = truncate( post.title, {
length: isPhotoOnly ? 50 : 140,
length: isPhotoOnly ? 500 : 140,
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 even need this now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that's to truncate the post title in photo cards, then I don't think so since we're doing it in css.

Copy link
Contributor

Choose a reason for hiding this comment

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

Dropped in 3d65c07.

@jancavan
Copy link
Contributor Author

One minor issue is that I sometimes see a flash of the standard card whilst the photo-only card is loading.

I noticed this too. The excerpts have completely been removed in photo cards, but the post title still flashes using the default card's style: https://cloudup.com/cTNFfmXaXTc

Example: http://calypso.dev:3000/read/feeds/23059853 (first post in stream)

@blowery
Copy link
Contributor

blowery commented Oct 19, 2016

@jancavan the font color was needed because the ellipsis is black otherwise...

@jancavan
Copy link
Contributor Author

@blowery My bad. Added it back: 915caec

@blowery
Copy link
Contributor

blowery commented Oct 19, 2016

I think the loading issue can be handled in a separate PR. We'll need to assign PHOTO_ONLY earlier i think.

@jancavan jancavan mentioned this pull request Oct 19, 2016
Closed
@bluefuton bluefuton force-pushed the add/reader/photo-only-card-refresh-stream branch from 99b8f8c to 3d65c07 Compare October 19, 2016 22:00
@bluefuton
Copy link
Contributor

These look lovely. Let's 🚢

screen shot 2016-10-20 at 11 10 48

@bluefuton bluefuton 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 Oct 19, 2016
@jancavan jancavan merged commit ae154b1 into master Oct 19, 2016
@jancavan jancavan deleted the add/reader/photo-only-card-refresh-stream branch October 19, 2016 22:15
@snyk-bot snyk-bot mentioned this pull request Mar 7, 2020
Open
@snyk-bot snyk-bot mentioned this pull request Mar 30, 2021
Open
@snyk-bot snyk-bot mentioned this pull request May 27, 2021
Open
@snyk-bot snyk-bot mentioned this pull request Sep 16, 2021
Open
@awaisab172 awaisab172 mentioned this pull request Jun 21, 2023
Open
@awaisab172 awaisab172 mentioned this pull request May 23, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blowery blowery blowery left review comments

@bluefuton bluefuton bluefuton left review comments

Assignees

@blowery blowery

@bluefuton bluefuton

@jancavan jancavan

Labels
[Feature] Reader The reader site on Calypso. [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
Reader Stream Refresh: Cards
Development

Successfully merging this pull request may close these issues.
5 participants
@jancavan @matticbot @bluefuton @fraying @blowery