-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
RichText: absorb internal list controls #10744
Conversation
Can we delete them here and now? |
Sure thing! More than happy to! |
There you go! |
d389235
to
2af3985
Compare
5d15b6f
to
ad64fcc
Compare
/> | ||
<RichTextShortcut | ||
type="primary" | ||
character="[" |
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.
These were ported backwards. ]
should be for the indent.
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.
These were ported backwards.
]
should be for the indent.
Updated in 25997a2
3d21666
to
bacbbdf
Compare
There's something not working quite right with indentation on new list items. I'll plan to investigate. |
It's not immediately obvious to me what's going on with the list indentation. The callbacks for the key handlers are not being called. Doing anything after the new item (even moving mouse, leaving screen and returning, pressing Escape) is enough to cause the keybind to work again, which itself is concerning as these don't seem like they should be causing any change in state to the RichText element. We also still didn't quite port the existing binds as they were before, because Cmd+M / Cmd+Shift+M were only bound previously if the keyboard didn't support the Given this is more a refactoring, and while I'd like to see the unstable functions gone sooner than later, I'm inclined to punt this to 4.4. |
@aduth Could you provide some steps to reproduce the indentation issue? I don't find any issues. |
@aduth Okay, I found the problem with the shortcuts. They unmount when the toolbar disappears. So when when you type, they no longer work, when you hover again so the toolbars show, they work again. |
bacbbdf
to
7d08d2e
Compare
It's a bit strange to me that we're only binding based on some random language detection. The [] shortcuts won't do anything if you don't have the keys on your keyboard so it doesn't hurt to bind them. I don't think we can accurately detect if a keyboard has these keys I also think we should bind Cmd+M / Cmd+Shift+M for everyone. Why would we only make them work in certain cases? |
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.
Looks good 👍
Thanks! |
Description
This change aims to absorb any RichText related logic from the list block into the RichText component. With this, none of our core blocks will use the
unstableGetSettings
orunstableOnSetup
props, which I would like to remove entirely so that no TinyMCE APIs remain exposed.Also fixes a bug where the the active button is not set for nested lists.
After #10799, the list buttons may be rewritten to manipulate the internal rich text value directly, removing the dependency on the TinyMCE lists plugin.
How has this been tested?
The list block should continue to work as before.
Screenshots
Types of changes
Checklist: