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

Create Core Plugin QueryCacheToSate #3221

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

felixfeng33
Copy link
Collaborator

@felixfeng33 felixfeng33 commented May 28, 2024

Description
QueryCacheToSate:

  • I'm not sure if caching in editor.apply is appropriate.
  • It may be necessary to add some conditions or comparison functions to optimize performance.

PlateEffects:

  • An id and a data-id have been added for obtaining the container dom Element.
  • I'm not sure if there's a better way to pass the container's DOM node to other plugins.

I will fix the test cases and add a changeset just before merging.

Copy link

vercel bot commented May 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plate ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 28, 2024 1:12pm

Copy link

changeset-bot bot commented May 28, 2024

⚠️ No Changeset found

Latest commit: b80d8b3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@felixfeng33 felixfeng33 requested review from 12joan and zbeyens and removed request for 12joan May 28, 2024 13:14

const { apply } = editor;

editor.apply = (operation) => {
Copy link
Member

Choose a reason for hiding this comment

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

I think editor.onChange is a better fit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've left some comments pertaining to this on Discord. https://discord.com/channels/1026227597115396188/1237021058067075072

};
};

export const withQueryCachToState = <
Copy link
Member

Choose a reason for hiding this comment

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

Let's name the plugin cache instead of queryCacheToState


export type QueryCachToSateEditor<V extends Value> = {
state: {
aboveEntries?: Generator<TNodeEntry<ENode<V>>, void, undefined>;
Copy link
Member

@zbeyens zbeyens May 28, 2024

Choose a reason for hiding this comment

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

let's reuse the same naming convention than Slate queries. For example Transforms.above -> above. For Plate exclusive, just try to follow the same pattern

import { createPluginFactory } from '../utils/createPluginFactory';

export type QueryCachToSateEditor<V extends Value> = {
state: {
Copy link
Member

Choose a reason for hiding this comment

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

cache may be better. What do you think @12joan ?

@@ -19,5 +19,9 @@ export function PlateEffects<
>({ children, ...props }: PlateEffectsProps<V, E>) {
usePlateEffects<V, E>(props);

return <>{children}</>;
return (
<div data-id={props.id} id="plate-editor">
Copy link
Member

Choose a reason for hiding this comment

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

is it needed?

Copy link
Collaborator Author

@felixfeng33 felixfeng33 May 28, 2024

Choose a reason for hiding this comment

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

toc Plugin need the container dom node to scroll to heading.I haven't thought of a good method to obtain container nodes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can get Slate's root contenteditable by passing the editor to toDOMNode.

export const KEY_QUERY_CACHE_TO_STATE = 'queryCacheToSate';

export const createQueryCachToStatePlugin = createPluginFactory({
key: KEY_QUERY_CACHE_TO_STATE,
Copy link
Member

Choose a reason for hiding this comment

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

let's add a boolean option for each query: don't run if false

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've left some comments pertaining to this on Discord. https://discord.com/channels/1026227597115396188/1237021058067075072

@felixfeng33 felixfeng33 marked this pull request as draft May 29, 2024 01:30
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.

3 participants