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

feat: add custom bylines #3667

Merged
merged 10 commits into from
Jan 17, 2025
Merged

feat: add custom bylines #3667

merged 10 commits into from
Jan 17, 2025

Conversation

chickenn00dle
Copy link
Contributor

@chickenn00dle chickenn00dle commented Jan 11, 2025

All Submissions:

Changes proposed in this Pull Request:

Closes https://app.asana.com/0/1208894036683495/1208945901567231 and https://app.asana.com/0/1208894036683495/1208945901567229

This PR adds a barebones custom byline feature to posts behind a NEWSPACK_BYLINES_ENABLED feature flag.

Screenshot 2025-01-13 at 19 17 44

How to test the changes in this Pull Request:

  1. Go to any post edit page
  2. Confirm a new Newspack Custom Byline panel is present in the post editor sidebar
  3. Enable the custom byline
  4. Enter some text and confirm the byline appears in the post editor above the content
  5. Add an author link to the byline by using the <Author id=USER_ID>Name</Author> tag
  6. Confirm a link appears in the byline
  7. Change the author ID to one that is not on the site
  8. Confirm the link disappears but the author name is still present in the byline
  9. Try to add any invalid tag (<Invalid>) and confirm an alert appears warning you only Author is supported
  10. Disable custom byline and confirm the byline disappears from the post editor
  11. Re-enable and confirm byline reappears as it was before

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@chickenn00dle chickenn00dle force-pushed the feat/add-custom-bylines branch from 7790cc7 to 5a259e9 Compare January 13, 2025 22:17
Comment on lines 59 to 80
const prependBylineToContent = text => {
const contentEl = document.querySelector( '.wp-block-post-content' );
if ( contentEl ) {
let bylineEl = document.getElementById( BYLINE_ID );
if ( ! bylineEl ) {
bylineEl = document.createElement( 'div' );
bylineEl.id = BYLINE_ID;
contentEl.insertBefore( bylineEl, contentEl.firstChild );
}
if ( isFetching ) {
bylineEl.innerHTML = __( 'Loading…', 'newspack-plugin' );
return;
}
// If there are author tags
if ( /<Author id=(\d+)>/.test( text ) ) {
console.info( 'Updating byline…' ); // eslint-disable-line no-console
text = text.replace( /<Author id=(\d+)>([^<]+)<\/Author>/g, ( match, authorId, authorName ) => {
const authorData = authors.find( a => a.id === parseInt( authorId ) );
return authorData ? `<a href="/author/${ authorData.name }">${ authorName }</a>` : authorName;
} );
}
bylineEl.innerHTML = text;
}
};
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 is another super hacky solution to getting the byline to render in the editor. Ideally we replace this when implementing the full feature.

@chickenn00dle
Copy link
Contributor Author

@leogermani, I've got this PR mostly ready for review assuming its enough to render the byline in the post editor only for the purposes of migration. I didn't want to do too much work on rendering this on the front-end without understanding the requirements fully here.

That said, if we'd rather figure this out now, I can get that added in the PR as well.

@chickenn00dle chickenn00dle marked this pull request as ready for review January 14, 2025 00:23
@chickenn00dle chickenn00dle requested a review from a team as a code owner January 14, 2025 00:23
@chickenn00dle chickenn00dle added [Status] Needs Review The issue or pull request needs to be reviewed and removed wip labels Jan 14, 2025
@chickenn00dle chickenn00dle force-pushed the feat/add-custom-bylines branch from 757b191 to 6b6de23 Compare January 14, 2025 00:25
Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

The link was not added for me att first. Then I noticed you were checking to see if the ID exists, but the fetch for authors in the useEffect only returns 10 authors, so it will never find the ID I was using.

But even when I used one of the returned IDs, the link was broken, with nothing after the /author/ part.

I don't think we need to verify anything at this point. Just create a link. And I think we can go siteurl.com/?author=$ID, no need to get their slugs.

Finally, let's also add a filter to the_content and render the byline on the front end as well. This will make QA easier

@chickenn00dle
Copy link
Contributor Author

Thanks for the review @leogermani!

I don't think we need to verify anything at this point. Just create a link. And I think we can go siteurl.com/?author=$ID, no need to get their slugs.

Sure thing! Updated in f8ec6b8

let's also add a filter to the_content and render the byline on the front end as well. This will make QA easier

Makes sense. Added in f8ceb44

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

I've added a few more things to the Readme. Thanks for the changes!

@github-actions github-actions bot added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Jan 17, 2025
@chickenn00dle chickenn00dle merged commit 3f45a6f into trunk Jan 17, 2025
8 checks passed
@chickenn00dle chickenn00dle deleted the feat/add-custom-bylines branch January 17, 2025 22:08
Copy link

Hey @chickenn00dle, good job getting this PR merged! 🎉

Now, the needs-changelog label has been added to it.

Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label.

If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label.

Thank you! ❤️

matticbot pushed a commit that referenced this pull request Jan 23, 2025
# [5.13.0-alpha.1](v5.12.2...v5.13.0-alpha.1) (2025-01-23)

### Bug Fixes

* add supported gateways check ([#3650](#3650)) ([74f7773](74f7773))
* **corrections:** replace deprecated sanitize method ([#3694](#3694)) ([ce50e24](ce50e24))
* remove support for legacy form checkout ([#3691](#3691)) ([46a3c16](46a3c16))
* **wcs:** expire manual subscriptions after on-hold duration ([#3681](#3681)) ([658416c](658416c))

### Features

* add custom bylines ([#3667](#3667)) ([3f45a6f](3f45a6f))
* rate limit checkout attempts ([#3678](#3678)) ([d275524](d275524))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.13.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Feb 3, 2025
# [5.13.0](v5.12.5...v5.13.0) (2025-02-03)

### Bug Fixes

* add supported gateways check ([#3650](#3650)) ([74f7773](74f7773))
* **corrections:** replace deprecated sanitize method ([#3694](#3694)) ([ce50e24](ce50e24))
* remove support for legacy form checkout ([#3691](#3691)) ([46a3c16](46a3c16))
* **wcs:** expire manual subscriptions after on-hold duration ([#3681](#3681)) ([658416c](658416c))

### Features

* add custom bylines ([#3667](#3667)) ([3f45a6f](3f45a6f))
* rate limit checkout attempts ([#3678](#3678)) ([d275524](d275524))
* **reader-revenue:** add PayPal Payments gateway to wizard ([#3665](#3665)) ([1476eed](1476eed))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 5.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @alpha released [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants