Skip to content

PCH Excerpt Suggestions: Add Persona and Tone settings #2776

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
Closed
acicovic wants to merge 49 commits into from
Closed

PCH Excerpt Suggestions: Add Persona and Tone settings #2776

acicovic wants to merge 49 commits into from

Conversation

@acicovic
Copy link
Collaborator

@acicovic acicovic commented Sep 12, 2024

Description

This PR adds Persona and Tone settings to our Excerpt Suggestions feature, in a similar manner to what already exists in our Title Suggestions feature.

Motivation and context

How has this been tested?

vaurdan and others added 30 commits August 22, 2024 14:23
@vaurdan
Add new Rest API base classes
9e074ae
@vaurdan
Implement the Content Helper API namespace
6ad9d44
@vaurdan
Add tests for the new REST API classes
7330236
@vaurdan
Update UI providers to use the new API endpoints
e500032
@vaurdan
Remove unused code and initializations
04de750
@vaurdan
Merge branch 'develop' into refactor/internal-api-content-helper
044b48d
@vaurdan
Fix sonarcloud issues
1b45964
@vaurdan
Merge branch 'develop' into refactor/internal-api-content-helper
f6f09ac
@vaurdan
Refactor to avoid consts and use getters instead
fe19315
@vaurdan
Fix minor docblock issues per code review
90c9808
@acicovic
Merge branch 'develop' into refactor/internal-api-content-helper
b000751
@vaurdan
Apply code review suggestions by @acicovic
77dbd2b
@vaurdan
Rename API parameter content to text
78395a7
@vaurdan
Add stats endpoint
a2cefb5
@vaurdan
Update the UI to use the new API endpoints
f1bb531
@vaurdan
Delete the old endpoints
fb3941a
@vaurdan
Add tests
5d785dd
@vaurdan
Apply code review suggestions by @acicovic
508b199
@vaurdan
Add stats endpoint
5e606da
@vaurdan
Update the UI to use the new API endpoints
89ef1bb
@vaurdan
Delete the old endpoints
e14f28c
@vaurdan
Add tests
5978a36
@vaurdan
Merge remote-tracking branch 'origin/refactor/internal-api-stats' int…
ce9be3a 
…o refactor/internal-api-stats

# Conflicts:
#	tests/Integration/RestAPI/ContentHelper/ContentHelperFeatureTestTrait.php
@vaurdan
Fix E2E test
fa5ee00
@vaurdan
Add settings endpoints
e252949
@vaurdan
Update UI to use the new settings API
36ade72
@vaurdan
Add tests
5b62a4e
@vaurdan
Remove old code
de7630a
@acicovic
Improve comments and whitespace
f06c489
@acicovic
Merge pull request #2731 from Parsely/refactor/internal-api-content-h…
313851e 
…elper

REST API Refactor: Base classes and Content Helper namespace implementation
@acicovic
Copy link
Collaborator Author

acicovic commented Sep 12, 2024

Notes:

  • This work is currently based on our refactor/internal-api branch in order to avoid duplicate work since it adds a settings endpoint.
  • @vaurdan, trying different things, I still couldn't make SettingsProvider to work. I suspect it might have to do with this being in Gutenberg Settings, and maybe a higher order component is needed as you've done previously for Smart Linking. Nothing has worked for me so far, but I'm probably missing something. Could you take a look?
  • This PR is currently incomplete due to the previous point.

@acicovic acicovic mentioned this pull request Sep 12, 2024
Closed
@acicovic
Copy link
Collaborator Author

acicovic commented Sep 12, 2024

@vaurdan: regarding my previous comment, let's also keep in mind that there's a feature request to have Excerpt Suggestions in the PCH Sidebar.

As this will probably affect how Excerpt Suggestions settings get saved, it would be good to factor this in during this work. If we're unsure of how this will play out, let's start implementing that request to avoid duplicate/refactoring work.

@acicovic acicovic mentioned this pull request Sep 17, 2024
Closed
@vaurdan
Copy link
Contributor

vaurdan commented Sep 24, 2024

@acicovic I went ahead and added the SettingsProvider to the component. It was a bit trickier, because of how we're using a SlotFill to insert the Excerpt panel to the Document sidebar (Using PluginDocumentSettingPanel).

This required a bit of reworking on how the code is organized - instead of being a standalone module, I made it part of the editor-sidebar module. This will also help when we add the panel to the PCH sidebar, since that move would have to be done nonetheless.

From your original code, instead of having a new settings namespace for the excerpt generator, I changed it so it's also part of the main SidebarSettings namespace.

Let me know your thoughts, and if you have any questions!

@vaurdan
Copy link
Contributor

vaurdan commented Sep 24, 2024

Oh - I also renamed the component from excerpt-generator to excerpt-suggestions on a different commit 🙂

@vaurdan vaurdan marked this pull request as ready for review September 26, 2024 09:42
@vaurdan vaurdan requested a review from a team as a code owner September 26, 2024 09:42
@vaurdan vaurdan mentioned this pull request Sep 26, 2024
Merged
acicovic
acicovic commented Oct 9, 2024
View reviewed changes
Copy link
Collaborator Author

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Thank you for completing this work! I agree with renaming ExcerptGenerator to ExcerptSuggestions anywhere possible.

I've left a couple of comments, the only really important one being the one about the filter.

69023030

This comment was marked as spam.

Sign in to view

@vaurdan
Copy link
Contributor

vaurdan commented Oct 16, 2024 •
edited
Loading

@acicovic I believe I addressed all your feedback! Let me know if you have any additional thoughts 🙂

I believe since you're the owner of this PR, you can't approve it, but if we have 🟢 , let me know and I can approve it myself.

acicovic
acicovic commented Oct 17, 2024
View reviewed changes
Copy link
Collaborator Author

@acicovic acicovic left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the filters situation!

I've left some minor comments.

@acicovic
Copy link
Collaborator Author

acicovic commented Oct 17, 2024

I've left a comment in our last discussion, but otherwise I think we're good to go. Let's not merge this though, until our API refactoring branch is merged first.

@acicovic acicovic added the Do not merge PR which shouldn't get merged yet label Oct 17, 2024
Base automatically changed from refactor/internal-api to develop October 25, 2024 11:13
@acicovic acicovic mentioned this pull request Oct 25, 2024
Merged
@acicovic
Copy link
Collaborator Author

acicovic commented Oct 25, 2024

Closing in favor of #2890.

@acicovic acicovic closed this Oct 25, 2024
@vaurdan vaurdan deleted the add/excerpt-suggestions-settings branch November 11, 2024 13:33
@acicovic acicovic removed the Do not merge PR which shouldn't get merged yet label Apr 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

+2 more reviewers

@vaurdan vaurdan vaurdan left review comments

@69023030 69023030 69023030 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vaurdan vaurdan

@acicovic acicovic

Labels

Changelog: Added PR to be added under the changelog's "Added" section Enhancement Ticket that requests an enhancement to an existing feature Feature: PCI Ticket/PR related to Content Intelligence

Projects

None yet

Milestone

3.17.0

Development

Successfully merging this pull request may close these issues.

Excerpt Suggestions: Allow different personas/styles

4 participants

@acicovic @vaurdan @69023030