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

[WIP] feat/489: Add Rewrite Tone feature #803

Draft
wants to merge 27 commits into
base: develop
Choose a base branch
from
Draft

Conversation

Sidsector9
Copy link
Member

@Sidsector9 Sidsector9 commented Sep 16, 2024

Description of the Change

Closes #489

Screencast

The Tone Rewriting options are preserved in local storage so that the admin does not have to perform the same selection every time.

Screen.Recording.2025-03-03.at.4.57.22.PM.mov

How to test the Change

Changelog Entry

Added - Rewrite Tone feature.

Credits

Props @jeffpaul @Sidsector9

Checklist:

@Sidsector9 Sidsector9 self-assigned this Sep 16, 2024
@github-actions github-actions bot added this to the 3.2.0 milestone Sep 16, 2024
@github-actions github-actions bot added needs:refresh This requires a refreshed PR to resolve. and removed needs:refresh This requires a refreshed PR to resolve. labels Sep 26, 2024
@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Nov 19, 2024
@dkotter dkotter modified the milestones: 3.2.0, 3.3.0 Dec 10, 2024
@dkotter dkotter modified the milestones: 3.3.0, 3.4.0 Feb 19, 2025
@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Mar 3, 2025
* @return {Array} Array of objects with content without delimiters.
*/
export const removeBlockDelimiters = ( content ) => {
return content.replace( /<!--[\s\S]*?-->/g, '' );

Check failure

Code scanning / CodeQL

Incomplete multi-character sanitization High

This string may still contain
<!--
, which may cause an HTML element injection vulnerability.

Copilot Autofix AI 9 days ago

To fix the problem, we need to ensure that the regular expression replacement is applied repeatedly until no more replacements can be performed. This will ensure that all instances of the targeted pattern are removed from the content, effectively sanitizing it.

The best way to fix this issue without changing existing functionality is to modify the removeBlockDelimiters function to apply the regular expression replacement in a loop until the content no longer changes. This approach ensures that all HTML comments are removed, even if they are nested or malformed.

Suggested changeset 1
src/js/utils/index.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/js/utils/index.js b/src/js/utils/index.js
--- a/src/js/utils/index.js
+++ b/src/js/utils/index.js
@@ -24,3 +24,8 @@
 export const removeBlockDelimiters = ( content ) => {
-	return content.replace( /<!--[\s\S]*?-->/g, '' );
+	let previous;
+	do {
+		previous = content;
+		content = content.replace( /<!--[\s\S]*?-->/g, '' );
+	} while (content !== previous);
+	return content;
 };
EOF
@@ -24,3 +24,8 @@
export const removeBlockDelimiters = ( content ) => {
return content.replace( /<!--[\s\S]*?-->/g, '' );
let previous;
do {
previous = content;
content = content.replace( /<!--[\s\S]*?-->/g, '' );
} while (content !== previous);
return content;
};
Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@Sidsector9
Copy link
Member Author

@jeffpaul I've updated the PR with a screencast showing how the UI will be for the admin. Can you please share your thoughts on it?

@dkotter
Copy link
Collaborator

dkotter commented Mar 3, 2025

@jeffpaul I've updated the PR with a screencast showing how the UI will be for the admin. Can you please share your thoughts on it?

I know I wasn't pinged but going to provide some thoughts anyway :)

Overall I think this looks good so no real complaints with this approach. What I do think is worth discussing is how best to ensure we have consistency across all of our different features.

As an example, right now someone could have a Generate titles button in the status panel and a Generate excerpt button in the excerpt panel and a custom ClassifAI panel with some options in it. Throw in this new experience and it starts to get a little confusing on where to go to manage things.

I don't think we need a single experience for everything as some things do make sense where they're at (for instance, I think the Generate excerpt button makes sense to stay). And I don't really have an answer here but bringing this up for discussion. Would it make sense to bring most of our options within this new plugin panel? Or are we okay with having things spread across multiple areas?

Also worth considering in progress features like #859 that adds a new button that opens a new modal. This feels like something that would be worth consolidating into what's done here, if we decide this approach is the right one

@Sidsector9
Copy link
Member Author

Sidsector9 commented Mar 4, 2025

Thanks for reviewing @dkotter

The main reason why this popup was introduced was to support multiple block selection of different types.
Gutenberg does not allow to add a Block Toolbar button in this case. In fact, as discussed with @jeffpaul, we're going to move the Content Resizing options from the Block Toolbar to this new popup as well, as we're looking forward to Resize Content across multiple blocks.

Once we move the Content Resizing options, we'll only have 2 different Plugin Panel areas – The sidebar and this new Popup in the editor header (unless we plan to re-introduce a custom Block Toolbar in the future).

Would it make sense to bring most of our options within this new plugin panel?

I've added a collapsible panel within the Popup for this reason, however that adds an extra step. If we move all settings here, then it'll require thorough testing, as Gutenberg doesn't really allow to extend this area. I've added the button in the header through some heavy workarounds and a custom Slot-Fill.

@10up/open-source-practice pinging ya'll to weigh in your opinions.

@dkotter
Copy link
Collaborator

dkotter commented Mar 4, 2025

I've added the button in the header through some heavy workarounds and a custom Slot-Fill.

So I know Gutenberg has the PluginSidebar SlotFill, which similarly adds a button to that top header. I know the experience there is slightly different (opens a custom sidebar rather than the popup you have now) but curious if that was explored as an option?

@Sidsector9
Copy link
Member Author

curious if that was explored as an option?

Yes. The PluginSidebar adds a custom Sidebar and a button for it to open/close the sidebar. We can't have a button there without the sidebar.

@dkotter
Copy link
Collaborator

dkotter commented Mar 4, 2025

curious if that was explored as an option?

Yes. The PluginSidebar adds a custom Sidebar and a button for it to open/close the sidebar. We can't have a button there without the sidebar.

To be more clear, my confusion was around this piece:

If we move all settings here, then it'll require thorough testing, as Gutenberg doesn't really allow to extend this area

I guess I would assume if we're using the PluginSidebar SlotFill, we'd be able to add whatever settings we would want into that sidebar without much issue (and I say all this without having looked at the code here so looking through that may answer my questions)

@Sidsector9
Copy link
Member Author

I guess I would assume if we're using the PluginSidebar SlotFill, we'd be able to add whatever settings we would want into that sidebar without much issue (and I say all this without having looked at the code here so looking through that may answer my questions)

This is possible. If we do this, would we also move controls for Excerpt and Title Generation too?

@dkotter
Copy link
Collaborator

dkotter commented Mar 4, 2025

This is possible. If we do this, would we also move controls for Excerpt and Title Generation too?

That's really the main question here that I'm bringing up for conversation as I'm not sure what's best. My concern is if someone has this Rewrite Tone Feature on plus Title Generation plus Excerpt Generation plus Classification each of those adds their own way to trigger functionality and wondering if we should be concerned with that fractured approach.

My own opinion (not saying we should make changes yet, just to be clear) would be to get rid of the custom panel we currently have and all the settings in there and add those to this new custom plugin section. I'd probably leave the Excerpt Generation button alone and maybe Title Generation as well but could also see benefit to putting those in the same place.

Ultimately I don't think any of that needs to happen on this PR, just want to ensure we're thinking through the best UI/UX for new Features like this and ensuring we're not backing ourselves into a corner with any decisions we make.

@jeffpaul
Copy link
Member

jeffpaul commented Mar 7, 2025

I talked through this a bit with @Sidsector9 earlier today and my knee-jerk reaction is:

  1. Any interactive element that is contextually alongside where an author/editor would otherwise be should stay as-is (e.g. Generate Excerpt, Generate Image).
  2. All other items should probably get moved into a new ClassifAI-specific PluginSidebar with the ClassifAI AI icon as the button for the top of the editor as there are a multitude of cases where someone may not immediately "see" the existing ClassifAI sidebar panel (e.g. the have the sidebar collapsed, they're in a cover block's style or settings) and having the ClassifAI AI button always present that would get them into the ClassifAI PluginSidebar regardless where they are in the editor. The one downside here is that could potentially mean at least one extra click (perhaps two) depending on where an author/editor happens to be in the editor and wants to interact with something from ClassifAI's PluginSidebar.

@fabiankaegy I'm curious if you might recommend something different/similar here?

@dkotter dkotter mentioned this pull request Mar 7, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite content to match brand standards
3 participants