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

POC - Migrating to Plate #575

Draft
wants to merge 34 commits into
base: main
Choose a base branch
from

Conversation

kudlajz
Copy link
Contributor

@kudlajz kudlajz commented Nov 13, 2024

This is the first part of the migration to Plate, replacing the main React component and all the code that depends on it with the Plate counterpart or Plate types.

For now, most of the existing plugins (extensions) have been left as-is so the PR doesn't become massive. This should also make sure that the editor behaviour stays the same.

One of the changes I've made has been to the plugins prop of the Editor component, which now accepts an array of PlatePlugin interface, allowing us to provide plugins from the outside, which is something we're planning to do eventually.

In the meantime, when this PR is in review and we can test the editor in Prezly, I'll work on refactoring the plugins to use Plate plugin approach and/or replacing them with existing Plate plugins to get rid of as much custom code as possible.

@kudlajz kudlajz self-assigned this Nov 13, 2024
@kudlajz kudlajz marked this pull request as draft November 13, 2024 19:10
@kudlajz kudlajz force-pushed the feature/dev-14744-explore-migrating-to-plate-editor branch from c150327 to b2d7993 Compare November 21, 2024 11:33
Comment on lines +11 to +12
"module": "esnext",
"moduleResolution": "bundler",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes were required for Plate packages to be able to be imported.

@kudlajz kudlajz marked this pull request as ready for review November 26, 2024 15:57
@digitalbase
Copy link

Surprised to see noone has commented or reviewed 😠

Copy link
Contributor

@e1himself e1himself left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! This was a lot to read in one go :D

Great job! So many code touched and converted to the new API! 🏆

I've left some comments/questions below. Some are just to not forget to address them before merging the code to master.

My main source of cognitive load in this PR was the distinction between Slate and Plate types, as well as different APIs seemingly doing the same thing. A few examples:

  • Element, Text and Node from slate vs TElement, TText and TNode from plate
  • Editor, Transforms, Node from slate vs editor.{method} or helper functions from plate
  • Editor vs ReactEditor vs SlateEditor vs PlateEditor

What are the differences? Should we always use the Plate's version? Is there a danger of using the original Slate's APIs? Can it cause problems with Plate?

packages/slate-commons/src/commands/toDomNode.ts Outdated Show resolved Hide resolved
packages/slate-commons/src/commands/toDomRange.ts Outdated Show resolved Hide resolved
packages/slate-editor/src/extensions/image/withImages.ts Outdated Show resolved Hide resolved
@@ -10,4 +11,4 @@ import type { NodeEntry, Range } from 'slate';
* keywords, where changes to the content (or some external data) has the
* potential to change the formatting.
*/
export type Decorate = (entry: NodeEntry) => Range[];
export type Decorate = (options: { editor: SlateEditor; entry: TNodeEntry }) => Range[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that the function interface should be (editor: SlateEditor, entry: TNodeEntry) => Range[] for consistency with other editor extension points below.


import type { HierarchyNormalizer } from '../types';

export function removeWhenNoChildren(): HierarchyNormalizer {
return (editor, node, path) => {
if ('children' in node && node.children.length === 0) {
Transforms.removeNodes(editor, { at: path, match: (n) => n === node });
if ((isEditor(node) || isElement(node)) && node.children.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isEditor(node)

I don't think it should be here. We never want to remove the editor node :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I didn't find a good way to narrow down the type so TS is satisfied. Will take another look

Copy link
Contributor

@e1himself e1himself Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But can you not do this?

if (isElement(node) && node.children.length === 0) {
    editor.removeNodes({ at: path, match: (n) => n === node });
    return true;
}

return false;

@@ -13,7 +18,7 @@ export function isEditorValueEqual<T extends Descendant>(editor: Editor, a: T[],
if (!isNodeText && !isAnotherText) {
const equal = editor.isElementEqual(node as Element, another as Element);
if (typeof equal !== 'undefined') {
if (editor.isVoid(node) || editor.isVoid(another)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why in other cases we switch from helper functions to editor.{method}() APIs, but in this case it's the opposite? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my only answer would be that it's easier to use editor.{method} because you don't have to import extra stuff?

Or, in case the editor.{method} does not exist, you import the specific function.

I don't remember exactly why I did it this way, but I'd say it's better to go with the editor.{method} approach for less overal imports.

Comment on lines +229 to +238
[
events,
withAutoformat,
withBlockquotes,
withCallouts,
withDivider,
withHeadings,
withLists,
withTextStyling,
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should plugins be in the dependency list?

@@ -108,8 +101,5 @@ export function getAllExtensions() {
}

export function createEditor(input: JSX.Element) {
return createBaseEditor(input as unknown as Editor, getAllExtensions, [
withEvents(events),
withNodesHierarchy(hierarchySchema),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure these two (or at least withNodesHierarchy()) are used in tests before we merge the PR.

Comment on lines +130 to +137
// withAutoformat
// withButtonBlocks
// withFloatingAddMenu={{
// tooltip: {
// placement: 'left',
// content: 'Add content to your story',
// },
// }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a note to either get this back to working, or remove before we merge the PR.

@kudlajz
Copy link
Contributor Author

kudlajz commented Dec 11, 2024

@e1himself

My main source of cognitive load in this PR was the distinction between Slate and Plate types, as well as different APIs seemingly doing the same thing. A few examples:

Element, Text and Node from slate vs TElement, TText and TNode from plate
Editor, Transforms, Node from slate vs editor.{method} or helper functions from plate
Editor vs ReactEditor vs SlateEditor vs PlateEditor
What are the differences? Should we always use the Plate's version? Is there a danger of using the original Slate's APIs? Can it cause problems with Plate?

I am still wrapping my head around the API, but from my understanding, I believe we should be using the Plate wrapper functions as those are the ones returning the TNode, TElement and TText types.

It gets a little bit confusing because when you use the shortcut editor.${method}, sometimes (or most times?) the method just calls the original Slate method directly which returns the base types like Node and Element. You have to use the Plate wrapper function like getNodeString which mimics Node.string but works with Plate's types.

When I was migrating the code, I didn't replace all the usages of Slate functions and types with the Plate counterpart because it would take too much time + it would also require reworking all the extensions as well, which would turn this large PR into even larger one.

So in most places I've only updated the API calls to satisfy TS, which seems like it's enough for the most part, but of course ideally we'll update everything along the way as we work our way through the extensions.

It's still a learning curve but should get clearer as we refactor more code.

@e1himself e1himself marked this pull request as draft December 11, 2024 11:37
@e1himself
Copy link
Contributor

I've converted it to draft to avoid accidentally merging :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants