-
Notifications
You must be signed in to change notification settings - Fork 43
Add the Title Generation UI #83
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
Add the Title Generation UI #83
Conversation
…styling on this and the toolbar to get things positioned correctly. Fix lint errors
…ng the executeAbility function
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.
I know, huge amount of line changes here but is needed to include all the various WordPress components we're using, otherwise linting will complain about importing things that don't exist
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #83 +/- ##
========================================
Coverage ? 29.19%
Complexity ? 142
========================================
Files ? 14
Lines ? 846
Branches ? 0
========================================
Hits ? 247
Misses ? 599
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
felixarntz
left a comment
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.
@dkotter Overall this is looking good, but a few concerns regarding coding standards and dependencies.
|
|
||
| const ExampleExperiment = () => { | ||
| return <div className="test">{ __( 'Example Experiment', 'ai' ) }</div>; | ||
| return <div className="test">{__('Example Experiment', 'ai')}</div>; |
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.
Why this "correction"? Before it was using WordPress Coding Standards, and now it isn't.
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.
We use wp-scripts here for linting (npm run lint:js which will run wp-scripts lint-js) and it was flagging this. I've always assumed the linting wp-scripts enforces is what WordPress wants (seems it uses https://www.npmjs.com/package/@wordpress/eslint-plugin) but if there's a better/more canonical approach to enforcing JS coding standards, happy to switch up our tooling for that
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.
That is really strange. I don't know if there was a recent update, but I've been using wp-scripts myself and it would always enforce the opposite (I.e. WordPress Coding Standards). Is there something else at play here maybe that leads to the regular Prettier taking over the WordPress specific version? 🤔
This needs some more digging because that's not something that should be changed without broader consensus by the Core developers. A few years ago there was actually a proposal to change it which ended up rejected.
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.
Yeah, not sure what's going on. We didn't have an ESLint config file so I was hoping adding that (done in f755562) would fix it. That did end up flagging additional errors (fixed in fc4d178) but still doesn't allow me to add extra spaces around things (I reverted the spacing change in that example file and you can now see the error it's flagging: https://github.com/WordPress/ai/actions/runs/19657884624/job/56298366699?pr=83#step:5:11).
I feel like I'm missing something simple here, a config file or dependency, but I'm not seeing anything obvious
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.
Is there maybe a conflicting version of prettier installed that's not the wp-prettier fork? If some dependency uses prettier, maybe that's what's happening.
@wordpress/scripts uses wp-prettier, so that's all as I thought it would be. But maybe another dependency is pulling in the "real" prettier, and that might cause the problem?
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.
Okay, think I got this fixed. Had to delete multiple dependencies (and the package-lock.json file) but finally got it installing correctly. Doing a quick search, seems the wrong version got installed in #73 though still not quite sure how. But I've got it working now and I've fixed up all lint errors so things should match proper coding standards now
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.
@dkotter LGTM!
There appear to be some transient PHPUnit CI failures though.
.prettierrc
Outdated
| @@ -0,0 +1 @@ | |||
| "@wordpress/prettier-config" | |||
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.
What does this do? Why is it needed?
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.
This loads in the WP prettier config to ensure those rules are used when running prettier. But this was part of me experimenting to get things fixed so I've reverted this to how it was initially setup, which removes this file from this PR: fb62804
JasonTheAdams
left a comment
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.
Great work, @dkotter!
Yeah, the PHPUnit tests are failing on Getting this error: |
Why don't we set it to 6.8 for now, so that we don't have these problems? It should still be fine to submit to .org like that. Bumping the minimum requirement to 6.9 probably takes 5 minutes and could be done on release day :) |
I went ahead and did that in 77ac717 and then updated #28 to include:
...as a precursor to submitting to WPORG. Assuming tests pass here, I'll merge this in. |
|
@dkotter perhaps a follow-up PR here to add a docs page like done in https://github.com/WordPress/ai/pull/155/changes#diff-f70665388390657902e73741dea03efcdadc4bd0ceb4432959f8903ae015c042? |
What?
Partially closes #10
Continues work on the Title Generation Experiment, with initial scaffold work done in #61 and the execution functionality done in #67.
Why?
We now have a Title Generation Ability that can be triggered to generate title suggestions but that requires you to make direct API requests. This PR adds in the UI that allows users to easily trigger this Ability from the post edit screen.
How?
Asset_Loaderclass within theTitle_GenerationExperiment to load the needed JS file when we're on the post edit screen for a post that has title support.TitleToolbarcomponent andTitleToolbarWrappercomponent (using typescript though can change to plain JS if we want)TitleToolbarcomponentTitleToolbarWrappercomponent as a plugin which will then find the title field and inject theTitleToolbarcomponentTesting Instructions
npm i && npm run build && composer install --no-devSettings > AI Credentialsand ensure you add at least one valid API keySettings > AI Experimentsand ensure the Title Generation Experiment is enabledGenerateorRe-generatedepending on if you have an existing title or notTesting Instructions for Keyboard
Screenshots
Test using WordPress Playground
The changes in this pull request can be previewed and tested using this WordPress Playground instance:
Click here to test this pull request.