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

CPT: Render basic post details in custom post type listing #4055

Merged
merged 5 commits into from
Mar 18, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Mar 15, 2016

This pull request seeks to continue iterating on the layout of the custom post type post listing screen, adding featured image, last edited date, a view button, and edit link to each post list entry.

Before After
Before After

Note: The layout is very much a work-in-progress and will continue to be refined over time.

Reference design: https://cloudup.com/cdHFJ0ddwl7

Implementation notes:

  • getSiteSlug must have been reimplemented, as while it does currently exist on the decorated site object, this is non-ideal and calculated values will be removed from the Redux store site state in the near future. See Site.prototype.updateComputedAttributes for the reference implementation and Framework: redux sites contains functions #2757 as the tracking issue for site state cleanup.
  • <PostTypePost /> will likely be split into separate components in a future iteration as each piece of the layout continues to be fleshed out.

Testing instructions:

  1. Navigate to My Sites
  2. If the current site does not have a custom post type enabled (e.g. Testimonials, Portfolio), switch to a site which has at least one enabled
  3. Click the custom post type navigation item in the sidebar
  4. Note that after posts have loaded, the layout reflects the "After" screenshot above for any posts which exist
  5. Note that the edit link (title), view link, updated date, and featured image are accurate

Caveats:

  • There is no loading indicator for posts
  • There is no warning shown if no posts exist, only a blank page

@aduth aduth added Posts [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Custom Post Types (CPT) labels Mar 15, 2016
}

return site.URL.replace( /^https?:\/\//, '' ).replace( /\//g, '::' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This differs from the current implementation in that hasConflict is not included: https://github.com/Automattic/wp-calypso/search?q=hasConflict

More background at 5836-gh-calypso-pre-oss - looks like this is intended to handle fairly extreme edge cases around Jetpack and WP.com sites ending up with the same URL, and the current implementation would need some changes to fit with a reduxified sites list. I personally think it would be ok to punt on it but wanted to mention.

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 differs from the current implementation in that hasConflict is not included: https://github.com/Automattic/wp-calypso/search?q=hasConflict

Good eyes. I must have had my blinders on when I had overlooked that part of the condition ( 🙈 ).

@enejb : There's quite a bit of background here. Do you think this is logic that needs to be preserved moving forward in the sites state? Apologies to shine light on this again, but I'm finding it difficult to find the conclusion of the previous discussion.

Copy link
Member

Choose a reason for hiding this comment

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

@aduth without it some users will not be able to access the site that they want. hasConflict is set when it sees that a .com site has the same url as a jetpack site. It sets the primary domain of the .com site to the something.wordpress.com even the site is domain mapped for example. This allow the user to navigate calypso without conflicts.

Since otherwise the user would not be able to land on the site that they want. Please keep it otherwise I am sure we will get someone telling us that they can't access the site.

@nylen
Copy link
Contributor

nylen commented Mar 17, 2016

Works as intended. Code looks good apart from the notes above.

One thing worth noting, but I doubt we can do anything about it, is that featured images still show up in the listing even if the theme doesn't do anything with them:

image

image
Source: https://jnylen0.wordpress.com/testimonial/

@aduth
Copy link
Contributor Author

aduth commented Mar 17, 2016

@nylen : Interesting, the theme's functions.php file does add support for post-thumbnails, and the /post-types endpoint for your site reports that the type does support thumbnails, though the /sites/%s endpoint claims that options.featured_images_enabled is false. Clearly the images aren't shown on the front-end of the site, but also curiously can be assigned in the wp-admin editor.

I've opened #4124 as a follow-up task for this, as there's much more to be fleshed out with regard to thumbnails in any case.

@aduth aduth force-pushed the update/cpt-type-details branch from 9af00e0 to bba3d2c Compare March 18, 2016 17:02
@aduth
Copy link
Contributor Author

aduth commented Mar 18, 2016

Rebased and added bba3d2c to account for site conflicts, including two new site state selectors getSiteCollisions( state ) and isSiteConflicting( state, siteId ). See also reference implementation.

@aduth
Copy link
Contributor Author

aduth commented Mar 18, 2016

@enejb : Would you mind reviewing the latest changes in bba3d2c to confirm that it covers the collisions needs well?

@aduth
Copy link
Contributor Author

aduth commented Mar 18, 2016

bba3d2c#commitcomment-16765942:

Looks great!
One case that still might cause a conflict is when we the .com site is set to https://example.com and the jetpack site is set to http://example.com. Which would result in the same slug. ( As far as I know ) which would prevent the user from visiting one of the urls on calypso.

Thanks for writing the tests.

@enejb : Good point, and one that doesn't seem to be accounted for in our current sites-list implementation. I've pushed d72d607 to ensure it's considered here.

@enejb
Copy link
Member

enejb commented Mar 18, 2016

Thanks for the fix!

@aduth aduth force-pushed the update/cpt-type-details branch from d72d607 to 437efe0 Compare March 18, 2016 18:16
@aduth aduth 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 Mar 18, 2016
aduth added a commit that referenced this pull request Mar 18, 2016
CPT: Render basic post details in custom post type listing
@aduth aduth merged commit 2093970 into master Mar 18, 2016
@aduth aduth deleted the update/cpt-type-details branch March 18, 2016 18:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants