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

1126 break up largo byline #1251

Merged
merged 50 commits into from Jul 18, 2016
Merged

1126 break up largo byline #1251

merged 50 commits into from Jul 18, 2016

Conversation

benlk
Copy link
Collaborator

@benlk benlk commented Jul 13, 2016

Changes

  • creates new class Largo_Byline to create a byline, in the new file inc/byline_class.php
    • adds outputting the author's avatar, a 32x32 image
    • adds outputting a link to the author's Twitter page
    • adds outputting the post's modified time, for signed-in users.
      • Should this criteria be changed?
    • changes the display of the "edit post" link to only show for users with the 'edit_published_posts' capability, where before it was merely 'edit_posts' capability. The distinction is that if a user cannot edit a published post, now they will not see the link.
  • changes largo_byline() to a wrapper around $byline = new Largo_Byline( $options );
  • extends Largo_Byline to support coauthors with Largo_CoAuthors_Byline
    • avatars, Twitter links, and organizations are supported for all coauthors
  • extends Largo_Byline to support Largo custom bylines with Largo_Custom_Byline
  • adds Largo_Custom_Byline and Largo_CoAuthors_Byline output to largo_byline() when situationally appropriate.
  • largo_byline() will aways return the byline's HTML now, where before it only returned if it wasn't echoing the byline
  • adds bare-bones styles for avatars in bylines, limiting them to the byline's expected line-height.
  • fixes bug in largo_get_avatar_src that was trying to use email addresses as user IDs

Why

Continuing on #1248 for #1126, this is the cleanup of that PR with requested changes. This replaces the action created in that PR with a series of classes, each only applied at the correct time.

Questions for review

  • should the post edited date be shown to all users?
  • are the labels of the byline okay?
  • what version number are we marking this with, in the docblock comments' @since tag?

benlk added 30 commits April 7, 2016 15:06
…rtions of the largo byline function out into separate functions
…wn components, for much the same reason as largo_byline was
…of errors.

Somehow, the array() passed in largo_byline() to the largo_byline action is being outright replaced by a string, a string that is the output of largo_byline_coauthors() or so it appears.

Here's the logged argument, and where it was throwing the error.

	[11-Jul-2016 18:25:05 UTC] '<span class="by-author"><span class="by">By</span> <span class="author vcard" itemprop="author"></span></span>'
	[11-Jul-2016 18:25:05 UTC] PHP Warning:  extract() expects parameter 1 to be array, string given in /vagrant/wp-content/themes/largo-dev/inc/post-tags.php on line 165
	[11-Jul-2016 18:25:05 UTC] '<span class="by-author"><span class="by">By</span> <span class="author vcard" itemprop="author"></span></span>'
	[11-Jul-2016 18:25:05 UTC] PHP Warning:  extract() expects parameter 1 to be array, string given in /vagrant/wp-content/themes/largo-dev/inc/post-tags.php on line 165
…t to largo_seo, for topical reasons"

This reverts commit f5bb75b.
@benlk
Copy link
Collaborator Author

benlk commented Jul 15, 2016

This is ready for review; the failing tests appear to be the usual batch of PHP 5.5 type-safety and WordPress 4.4+ taxonomy term changes.

@benlk benlk removed their assignment Jul 15, 2016
* If job titles are enabled by Largo's theme option, display the one for this author
*/
function job_title() {
$show_job_titles = of_get_option( 'show_job_titles' );

Choose a reason for hiding this comment

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

of_get_option should always have a default value set

@aschweigert
Copy link

@benlk just those couple small things, but otherwise this lgtm. nice work on this. huge improvement.

@benlk benlk merged commit 3076718 into develop Jul 18, 2016
@aschweigert aschweigert deleted the 1126-break-up-largo_byline branch September 21, 2016 01:00
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.

None yet

2 participants