-
Notifications
You must be signed in to change notification settings - Fork 32
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
PCH Excerpt Suggestions: Add Persona and Tone settings #2776
Conversation
…o refactor/internal-api-stats # Conflicts: # tests/Integration/RestAPI/ContentHelper/ContentHelperFeatureTestTrait.php
…elper REST API Refactor: Base classes and Content Helper namespace implementation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (4)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Notes:
|
@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 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 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! |
Oh - I also renamed the component from |
There was a problem hiding this 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.
src/content-helper/editor-sidebar/excerpt-suggestions/class-excerpt-suggestions.php
Outdated
Show resolved
Hide resolved
src/content-helper/editor-sidebar/excerpt-suggestions/class-excerpt-suggestions.php
Outdated
Show resolved
Hide resolved
…ent_helper_excerpt_generator`
@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. |
There was a problem hiding this 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.
src/content-helper/editor-sidebar/class-editor-sidebar-feature.php
Outdated
Show resolved
Hide resolved
src/content-helper/editor-sidebar/excerpt-suggestions/class-excerpt-suggestions.php
Show resolved
Hide resolved
src/content-helper/editor-sidebar/excerpt-suggestions/class-excerpt-suggestions.php
Outdated
Show resolved
Hide resolved
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. |
Closing in favor of #2890. |
Description
This PR adds
Persona
andTone
settings to ourExcerpt Suggestions
feature, in a similar manner to what already exists in ourTitle Suggestions
feature.Motivation and context
How has this been tested?