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

feat: custom byline interface #3746

Open
wants to merge 51 commits into
base: trunk
Choose a base branch
from
Open

feat: custom byline interface #3746

wants to merge 51 commits into from
+581 −49

Conversation

allysonsouza
Copy link
Collaborator

@allysonsouza allysonsouza commented Feb 12, 2025 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

This Pull Request implements the feature to allow the selection of author tokens from authors selected on Co-Authors Plus to compound a custom byline with them.

How to test the changes in this Pull Request:

  1. Go to a post edit page and assign authors to the post on co-authors plus panel
  2. Below it, you will see the Newspack Custom Byline panel. Set the toggle to active to enable custom byline.
  3. You will see the custom byline field, which is a div with contenteditable. Below it you should see the authors assigned to the post as buttons that will insert the tokens on the content when clicked.
  4. Click on buttons to add the token to the content
  5. Click on buttons inside the byline content to remove token
  6. Also, the tokens should be removed when editions are made to then with delete/backspace for example
  7. Save the post, reload the page and see if the custom byline saved is what is expected
  8. Check if the custom byline is editable as expected too after saving it

Things to check:

  • The authors set on Co-Authors Plus are being displayed as tokens to be used on Custom Byline?
  • When I click on token "+", the author token is inserted at the end of the byline?
  • Does the token get removed after a change on it's content is made (by using delete, backspace or by adding additional characters)?
  • Does the token get removed when I click on "x" on it after inserted into the byline?
  • Does additional space after the token is inserted to allow more text to be typed after it?
  • Does the byline get save properly?
  • After saving, does the available tokens (the ones that aren't in use) are correctly displayed below the byline field?

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?

@allysonsouza allysonsouza self-assigned this Feb 12, 2025
@allysonsouza allysonsouza changed the title Feat: custom byline interfce feat: custom byline interfce Feb 12, 2025
@allysonsouza allysonsouza added the [Status] Needs Review The issue or pull request needs to be reviewed label Feb 13, 2025
@allysonsouza allysonsouza marked this pull request as ready for review February 13, 2025 18:44
@allysonsouza allysonsouza requested a review from a team as a code owner February 13, 2025 18:44
claudiulodro
claudiulodro requested changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@claudiulodro claudiulodro left a comment

Choose a reason for hiding this comment

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

Overall I think this is coming together! The UX of it feels good. I noticed a few quirks in my testing:

  1. In general it's kind of hard to read once there are a few authors in there:
Screenshot 2025-02-13 at 1 29 49 PM

Not sure if we should reduce the size of the author boxes or how best to handle that. What do you think, @thomasguillot?

  1. Using the arrow keys, you can go into one of the boxes and then type to modify the content. I think it should not be editable.
Screenshot 2025-02-13 at 1 31 01 PM
  1. If you select a portion of the text that includes a box and delete, the box doesn't go back into the available authors section:
Screenshot 2025-02-13 at 1 33 24 PM Screenshot 2025-02-13 at 1 33 34 PM
  1. Authors are always inserted at the end of the text box. I think the best experience would be for them to be inserted at the cursor position, otherwise it's tricky to edit existing bylines to add/change authors around.

@leogermani
Copy link
Contributor

This is starting to look pretty good!

Here are two things I noticed:

  • This feature should also work without CoAuthors Plus. In that case, it would list only the normal post author
  • The input should be pre-filled with the default by byline
  • The value stored in the post meta field should follow what's described here

The idea of storing the ID and the name is to have a fallback in case the user is deleted. When we render the byline, we'll always read the ID and get the Display name from the database, but if the ID is gone, we'll use the name that is stored there.

@leogermani
Copy link
Contributor

Ah, and before you get crazy and go deep in a rabbit role, we DON'T NEED TO SUPPORT Guest Authors

@claudiulodro claudiulodro added [Status] Needs changes or feedback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Feb 13, 2025
@thomasguillot
Copy link
Contributor

Overall I think this is coming together! The UX of it feels good. I noticed a few quirks in my testing:

  1. In general it's kind of hard to read once there are a few authors in there:
Screenshot 2025-02-13 at 1 29 49 PM Not sure if we should reduce the size of the author boxes or how best to handle that. What do you think, @thomasguillot?

You're right, this doesn't look good. Let me think about a different approach here.

@thomasguillot
Copy link
Contributor

Note: The panel should simply be named "Byline", not "Newspack Custom Byline"

@thomasguillot
Copy link
Contributor

Can we please make sure the style matches Core's FormTokenField

Also we can use @wordpress/base-styles/colors rather than hardcoding the values.

@miguelpeixe
Copy link
Member

I've just tested on a fresh post and I still have issues with the backspace interaction:

custom-byline.mov

Another problem I faced while drafting the new post was that the additional authors only became available after I saved and refreshed the post. Ideally, the component would be aware of the edited changes and update the component's available tokens.

@allysonsouza
Copy link
Collaborator Author

The latest commit fixes the backspace interaction, @miguelpeixe. About authors, it was expected that for the first version that would be acceptable (to have to refresh the page to make them available to use on byline), can you confirm that, @claudiulodro?

@allysonsouza allysonsouza changed the title feat: custom byline interfce feat: custom byline interface Feb 25, 2025
@miguelpeixe
Copy link
Member

Thanks for the revisions @allysonsouza!

Backspace interaction is working well for deleting the token when the cursor is at the end of it. I'm facing a couple of issues still:

  • The cursor repositions to the beginning after deleting a token
  • Removing (via delete or backspace) multiple selected text doesn't restore the token
bylines.mov

leogermani
leogermani reviewed Mar 18, 2025
View reviewed changes
goldenapples
goldenapples reviewed Mar 18, 2025
View reviewed changes
@allysonsouza
Copy link
Collaborator Author

After struggling to handle the state of the content, which was not being managed by React state, @goldenapples started working on a simplification to use onInput instead of MutationObserver, which lead to a simpler and shorter code.

He's proposing it on this PR on our fork, I've been testing it and it seems like we need to deal with the removal of the tokens itself when there's a change to the token content (like adding characters or removing them with backspace or delete).

That seems possible to be handled by onInput too, by adding an onInput on each token to remove itself when changes happen and to keep the onInput on the wrapper contenteditable div.

@goldenapples goldenapples force-pushed the feat/byline-interface branch from b49dbc5 to 0867dfa Compare March 25, 2025 12:22
allysonsouza and others added 23 commits March 25, 2025 15:15
@allysonsouza @goldenapples
feat: make custom bylines to work without co-author plus activated
de13264
@allysonsouza @goldenapples
feat: make custom byline field to be displayed on a modal
c7cd705
@allysonsouza @goldenapples
fix: mystype newspackByline.is_co_authors_plus_active
8ecca9f
@allysonsouza @goldenapples
fix: adding space after token insertion at the end of byline
6e8898a
@allysonsouza @goldenapples
fix: improve byline innerHTML assignment
d7691f1
@allysonsouza @goldenapples
fix: typo isTokensdReady
8e03133
@leogermani @goldenapples
docs: update docs
d0b1e1c
@allysonsouza @goldenapples
fix: remove non used authors query
108d7e7
@allysonsouza @goldenapples
fix: cubic-bezier transition typo
2c00941
@allysonsouza @goldenapples
fix: remove updateBylineMeta from click on x/removal callback event
03d4100
@allysonsouza @goldenapples
fix: remove unnecessary "backwards" compatibility since it's new feature
cab5d82
@goldenapples
refactor: replace MutationObserver with event listeners
6fede99 
Replaced the MutationObserver with an event listener, replaced some of
the DOM interaction with a useRef element, and tried to refactor some of
the data getters and setters.
@goldenapples
fix: update effect registration so it only runs on mounting
36b097e
@goldenapples
refactor: remove unneded "remove token" handler
aa4087a
@goldenapples
fix: look up the byline meta on mount, not just on initial render
13a1d33
@goldenapples
fix: set contenteditable="false" on token spans
3d17eee 
Prevents editing the content in these spans, and makes it so that
deleting (with backspace) will remove the whole span at once rather than
editing within the span.
@goldenapples
chore: add some missing docblocks and fix some lint issues
fa68c1c
@goldenapples
fix: set default insert location to end of editable field
5323909 
Updates the "cursorPos" variable logic to use the end of the line if
there's no cursor position set yet, and to add a non-breaking space at
the end of the text if necessary. Also updates the white-space styling
of the editable area, to ensure that users can select any point in the
text they may need to.
@allysonsouza @goldenapples
fix: remove margin from last token to prevent confusion with whitespace
5ebb183
@goldenapples
feat: add preview to sidebar interface
07c5bcf
@allysonsouza @goldenapples
fix: rename panel to Byline
3e51b6b
@allysonsouza @goldenapples
fix: make modal width fixed
75218f7
@allysonsouza @goldenapples
feat: add function to parse meta for preview
d403417
@goldenapples goldenapples force-pushed the feat/byline-interface branch from ef23903 to d403417 Compare March 25, 2025 19:18
@leogermani leogermani mentioned this pull request Mar 26, 2025
Merged
6 tasks
@leogermani
Copy link
Contributor

Heads up that I've implemented the data structure changes that this PR does in trunk to allow us to move on with some tests. This might create conflicts that should be easily resolved - just ignore the changes on trunk

#3863

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goldenapples goldenapples goldenapples left review comments

@leogermani leogermani leogermani left review comments

@claudiulodro claudiulodro Awaiting requested review from claudiulodro

Requested changes must be addressed to merge this pull request.

Assignees

@allysonsouza allysonsouza

Labels
[Status] Needs changes or feedback The issue or pull request needs action from the original creator
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@allysonsouza @leogermani @thomasguillot @miguelpeixe @goldenapples @claudiulodro @joeleenk