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

feat: add textResolver #35

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

Conversation

alexander-lozovsky
Copy link

@alexander-lozovsky alexander-lozovsky commented Aug 9, 2023

#67

Add textResolver to control the rendering of the plain text, to handle cases when you need to preprocess the text before rendering (translation, sanitization, etc)

lib/src/types.ts Outdated Show resolved Hide resolved
lib/RichTextRenderer.astro Outdated Show resolved Hide resolved
lib/src/utils/resolveRichTextToNodes.ts Outdated Show resolved Hide resolved
@edvinasjurele
Copy link
Collaborator

Also do not forget DCO, thus all commits has to be signed-off. Read the details and follow instructions.

@edvinasjurele
Copy link
Collaborator

Lets also update README with the information about the usage, and maybe some use cases.

Optionally, we should extend DEMO app with the example of such resolver too. We can do some simple text replacement example or similar simple case just to illustrate that it is possible to hook into text node and affect it.

@edvinasjurele edvinasjurele marked this pull request as draft September 12, 2023 19:47
@alexander-lozovsky alexander-lozovsky force-pushed the feat/add-string-resolver branch 3 times, most recently from 0a6b348 to fb8ee60 Compare November 2, 2023 14:02
@alexander-lozovsky alexander-lozovsky marked this pull request as ready for review November 2, 2023 14:05
@alexander-lozovsky alexander-lozovsky changed the title feat: add stringResolver feat: add textResolver Nov 2, 2023
content: text.replace("{name}", "World"),
component: "span",
props: { class: "class-2" },
}),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, this does look weird honestly, and I am sure it clashes with the existing API. The problem is that conceptually the textResolver should be meant for text resolving only, and should not return ComponentNode at all. This function is not about the node logic at all, but rather about manipulating the content property in whatever the node is.

to get same result you have described with solution we should better have the following API:

     schema: {
        nodes: {
          text: () => ({
            component: "p",
            props: { class: "class-1" },
          }),
        },
      },
      textResolver: (text) => text.replace("{name}", "World")

(the textResolver extends the capability of the node, but does not override it)

Alternatively, we can expect the shape of textResolver?: (str: string) => Pick<ComponentNode, 'content'>;
and the usage:

 textResolver: (text) => ({
    content: text.replace("{name}", "World"),
 }),

but again, this may misslead users that it is not the whole component node but rather just a content, so might be bad alternative.

What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

it should return ComponentNode for cases when you example you want to support ICU syntax. So in this case all the texts need to be rendered inside some .astro component

I can suggest to support both outputs:
textResolver?: (str: string) => ComponentNode | string;

Signed-off-by: Alexander Lozovsky <[email protected]>
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.

2 participants