Reader: Add new thumbnail and excerpt component for Refresh #8463
Conversation
|
To dos:
|
|
Just a sample for the gallery card would be enough I think. Or we can leave it for another PR. |
|
We're going to just create separate PRs for each card type instead of adding them all in this PR:
|
| <li className="reader-post-card__visit"> | ||
| <ExternalLink icon={ true }>Visit</ExternalLink> | ||
| </li> | ||
| <li className="reader-post-card__share"> |
There was a problem hiding this comment.
When #8568 lands, we can replace the buttons here with ReaderPostActions.
| .reader-share__button-label, | ||
| .comment-button__label-status, | ||
| .like-button__label-status { | ||
| .is-group-reader .reader-share__button-label, |
There was a problem hiding this comment.
@bluefuton I had to scope this to .is-group-reader because it trumps the styles that I'm assigning to the action labels.
|
This addresses (HTML/CSS): #8515 |
53423ce to
a063c6b
Compare
|
@jancavan @blowery is http://calypso.localhost:3000/devdocs/blocks/refresh-card the right place to be reviewing this? The new card style isn't appearing in my Following stream, although I see some card factory logic that suggests that's the plan? |
|
@bluefuton Yes, it's on http://calypso.dev:3000/devdocs/blocks/refresh-card. Not sure why you're not seeing it? |
|
Thanks @jancavan - yep, I'm seeing it there, but as per your screenshot I saw the action buttons overlapping the bottom of the card. I assume that's alright because the new stream is on a white background? If so, should we add some more padding to the container card in devdocs perhaps? |
|
Working on getting it into the streams. Turns out we're not actually using |
This is exactly why. But looking at the mockups, they do appear narrower so I'm going to try adding a padding. Good call. |
|
In 607bd8d, made sure content width is So added a bit more padding just so these cards look more presentable on Devdocs: |
|
Thanks for the feedback @fraying! I suggest we address all of that in a followup PR and get this one 🚢 - then we can drop in the new action links component too. |
| <a className="reader-post-card__link">catsandfurballs.wordpress.com</a> | ||
| <div className="reader-post-card__timestamp-and-tag"> | ||
| <span className="reader-post-card__timestamp">1 hour ago</span> | ||
| <span className="reader-post-card__tag"><Gridicon icon="tag" />Pets</span> |
There was a problem hiding this comment.
Is there a way we can eliminate this hard-coded data so it doesn't get deployed by accident to production? If it's only here for development purposes, it seems like we could at least make it explicitly test data, passing it in as a prop with the fake data standing out in the file
| ]; | ||
|
|
||
| const RefreshCards = React.createClass( { | ||
| displayName: 'RefreshCard', |
There was a problem hiding this comment.
This displayName isn't needed because we are assigning the component to a variable name, in which case it's automatically inferred
| } | ||
| ]; | ||
|
|
||
| const RefreshCards = React.createClass( { |
There was a problem hiding this comment.
since this is new code and Facebook has chosen to move away from createClass(), do you think we could convert this? it looks like it's a good candidate for a functional stateless component.
| const isPhotoOnly = post.display_type & DisplayTypes.PHOTO_ONLY; | ||
| const title = truncate( post.title, { | ||
| length: isPhotoOnly ? 50 : 140, | ||
| separator: /,? +/ |
There was a problem hiding this comment.
is the space inside the regex a typo or is it only supposed to split on either a , or more than one ? there is the\s character class to indicate space characters…
| </li> | ||
| <li className="reader-post-card__comments"> | ||
| <CommentButton | ||
| commentCount={ post.discussion.comment_count } |
There was a problem hiding this comment.
Could we get a post without a discussion property? if so, we could fail at runtime. How about either filtering out those invalid post objects before we get here or use a safe-defaulting getter such as this:
import { get } from 'lodash';
commentCount={ get( post, 'discussion.comment_count', 0 ) }| postId={ post.ID } | ||
| tagName="span" | ||
| showZeroCount={ true } | ||
| showLabel={ true } /> |
There was a problem hiding this comment.
Let's check on the indentation here. Typically we start indenting the attributes one more than the component
<LikeButton
siteId={ post.site_ID }
…
/>(also applies elsewhere but I only wanted to leave a single comment)
|
|
||
| @include breakpoint( "<480px" ) { | ||
| padding-left: 0; | ||
| } |
There was a problem hiding this comment.
I'm not su
10000
re how far this applies, but our coding guidelines dictate that we should be mobile-first, in which case padding-left: 0 should be the default and the 265px modifier should be allocated to some > breakpoint
|
|
||
| onCommentClick() { | ||
| this.props.handleClick && this.props.handleClick( this.props.post, { comments: true } ); | ||
| } |
There was a problem hiding this comment.
we've been starting to use class properties in lieu of binding in the constructor. it can be helpful in places like this…
onClick = () => this.props.handleClick( this.props.post );we get the appropriate this binding without ever declaring the bind()
also note that I skipped checking for the presence of this.props.handleClick. we can do this safely by defining a default prop…
import { noop } from 'lodash';
static defaultProps = {
handleClick: noop,
};| let cardFactory = null; | ||
| if ( config.isEnabled( 'reader/refresh/stream' ) ) { | ||
| cardFactory = refreshCardFactory; | ||
| } |
There was a problem hiding this comment.
this could be an immutable assignment
const cardFactory = config.isEnabled( 'reader/refresh/stream' )
? refreshCardFactory
: null;| @@ -459,8 +461,10 @@ module.exports = React.createClass( { | |||
| showingStream = true; | |||
| } | |||
|
|
|||
| const StreamMain = config.isEnabled( 'reader/refresh/stream' ) ? ReaderMain : Main; | |||
There was a problem hiding this comment.
usually we want to put each item in the ternary on a new line unless it's very small. this case may fit but I think it could be a bit better on three lines…
const StreamMain = config.isEnabled( 'reader/refresh/stream' )
? ReaderMain
: Main;|
Hi @jancavan - I popped in for a post-facto review because I noticed that the reader is now broken in master - all of the post authors are Immediately we need to fix the reader in master if you ask me though: priority # 1. Please feel free to reach out to me if you have any questions about this or about my comments. I'd love to be able to be a help. |
|
Here is a quick fix for Set |
|
What dev is this "blocking"? |
|
@blowery to whom and about what are you asking your question? I couldn't find any reference to "blocking" above… |
|
@dmsnell i think i was conflating the "immediately" with "blocking". too early, not enough ☕️ :) |
|
@dmsnell I appreciate the review (and there's some useful feedback there, thank you). I'm curious about the same thing as @blowery I think: in what respect was having this feature flag enabled in development only "disrupting everyone else doing development work"? Was it because you thought the Reader was broken in other environments, so you went to investigate? |
right @bluefuton - I logged in and noticed that every post in the reader view was showing invalid data. this started a chain reaction of questing if I had broken it, if it was broken in production, if it was broken in master, asking people and getting others involved in the Slack channel, finding that it was intentionally broken, and dealing with it. maybe this sounds dramatic here the way I wrote it, but several of us ended up wasting a bunch of time and adrenaline because we thought we had an emergency. probably since the reader is at the forefront of everything in Calypso it stunned us more than some small feature on the periphery. we ended up doing almost the same amount of work to discover that this was intentional as we would have done to fix it had it been a bug. that's why I used the word "disruption" it ended up not being broken in production, and it ended up being something at least a couple people were aware of, but to the rest of us it was kind of all-hands-on-deck when we saw it. I think in cases like this where something so visible as the author of posts in the reader view get mangled, some clear communication on the p2 and in Slack would be a minimum requirement, if not making it work before merging. I'm sure this is a big feature being broken into multiple parts, but my general rule is to try hard and not merge anything that wouldn't be right in production. in this case I'm not sure how much extra effort it would have required to get the author and blog mapping right before merging or how many other things are affected by the new layout, but as big as the PR already was it seems like it could have fit in there (or ideally to have been broken up even further into separate and incremental PRs) I appreciate all your work and don't want to cast a shadow or any sense of blame or shame here. in the future I think there are probably some better ways to merge in changes without risking these kinds of disruptions. ❤️ |
|
I appreciate the thoughtful response @dmsnell. We're sorry if the changes caused a panic! Like you I'm a fan of small PRs, and this one is a bit bigger than we'd like, but it lays the foundations for a series of smaller PRs implementing the Reader Stream Refresh (https://github.com/Automattic/wp-calypso/projects/10). To get the cards to the point where they'd be right in production would make this PR pretty enormous. For example, sorting out the author and site name in the header (the issue that originally caused the alarm) is handled in #8701, currently running at 163 lines on its own. The feature flag enables us to break the project in smaller pieces but the tradeoff is we have to accept that, early on, there may be work going into master that isn't ready to be seen in production yet. In terms of changes for next time, I think we should have kept the feature flag turned off by default until the new stream cards were in reasonable shape. We've now made that change and are using Thanks again for your feedback :) |
Thanks @bluefuton - I like disabling it until it's ready. Another idea I had is something I've done before; namely to post a big alert box above the UI indicating that this is a work-in-progress. For example, had the reader loaded with a card at the top that indicated it was staging-only and in-progress, that would have also prevented the panic. I like your solution better though 😄 |
Uh oh!
There was an error while loading. Please reload this page.