-
-
Notifications
You must be signed in to change notification settings - Fork 746
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
Fix slate RichTextwidget to allow support slate extensions refs#6570 #6586
Conversation
✅ Deploy Preview for plone-components canceled.
|
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 glanced at this but I'm not familiar with slate extensions, so don't really feel competent to review it. @nileshgulia1 It would help if you could say a bit about what you're trying to do here or how to test it. The lack of an automated test also makes me more hesitant to click approve.
@davisagli Its something which we are also doing it for textBlock. slate-extensions just adds extra capabilities to slate wrapped components. To test this, you can create a RichText widget in your content type, try to create some lists(ul) with few li's, save the page and re-edit them, you see there is no way to get outside of lists, or actually there is no way to add anything else. Perhaps I should create some E2E tests also 🤔 |
There is a detached mode also but I think its for blocks not widgets. |
@nileshgulia1 I don't follow. I can use the up and down arrows to move between blocks. Screen.Recording.2025-01-28.at.2.35.25.PM.mov |
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.
@nileshgulia1 @stevepiercy I think I understand the problem better now:
- This is specifically a problem with the rich text widget, not the slate block. To test it, you have to add a rich text field to a content type, and edit there, not in a block.
- The problem is that when you're typing in a list, you would expect to be able to hit Enter a couple times to end the list. But this just creates new list items, and there's no obvious way to add text that is not in the list.
Do I have it right? If so, I'd suggest a different changelog entry here: "Fix typing Enter to end a list in the RichTextWidget." Using a slate extension for this is just an implementation detail, not the point of the change.
Unfortunately, I tested with this PR locally and it doesn't actually seem to fix the problem.
I'm curious as it does work for me with a backend different from Screen.Recording.2025-01-29.at.2.28.31.PM.mov |
@nileshgulia1 Your video looks like what I would expect! I'll try to step through in the debugger in my local site later this week and see if I can tell why it didn't work for me. |
@@ -127,7 +132,10 @@ const HtmlSlateWidget = (props) => { | |||
block={block} | |||
selected={selected} | |||
properties={properties} | |||
extensions={slateWidgetExtensions} |
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 fixed the issue with inconsistency, htmlSlateWidget is used as richtext
and we override this in our policy adddon. Thus, the same extension should be ported here as well.
I will write the cypress for this scenario.
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.
-cc @davisagli
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.
@nileshgulia1 Thanks, now it does what I expected!
@sneridagh We can merge this please. Thank you. |
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.
@nileshgulia1 Use .internal
for changes that only involve tests. I updated it already.
Closes #6570