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

[Feature Request] Pass default resolver as parameter #104

Open
apfelbox opened this issue Sep 30, 2024 · 3 comments
Open

[Feature Request] Pass default resolver as parameter #104

apfelbox opened this issue Sep 30, 2024 · 3 comments
Assignees
Labels
feature [Issue] New feature or request p2-nice-to-have [Priority] Lower priority, beneficial enhancements that are not urgent. pending-author [Issue] Awaiting further information or action from the issue author

Comments

@apfelbox
Copy link

When customizing the resolver of an element, it would be great to get the default resolver passed in.
Because some time you may only want to add some attributes, but keep the remaining logic intact.
To reuse the example from your announcement post:

const html = richTextResolver({
  resolvers: {
    [MarkTypes.LINK]: (node, defaultResolver) => {
      node.attrs.class = "text-blue-500 hover:text-blue-700 underline transition-colors duration-200 ease-in-out";
      return defaultResolver(node);
    },
  },
}).render(doc);

To be fair, with the current structure that is kind of inconvenient, as the attrs and the node are combined, but it is still possible.

This would be way more readable if it looked like this:

const html = richTextResolver({
  resolvers: {
    [MarkTypes.LINK]: (tag, attrs, children) => {
      return defaultResolver(
        tag,
        {...attrs, class: "text-blue-500 hover:text-blue-700 underline transition-colors duration-200 ease-in-out"},
        children,
      );
    },
  },
}).render(doc);

But maybe that is out of scope.


Expected Behavior

It would be great to have this feature.

Current Behavior

Not implemented yet

Steps to Reproduce

n/a

@apfelbox apfelbox changed the title [Feature Request] Pass default parameter as parameter [Feature Request] Pass default resolver as parameter Sep 30, 2024
@markus-gx
Copy link

I guess I just add a comment here, since I have a nearly identical Feature request.

I have a scenario where I need to shift the heading levels (h1, h2, etc.) by n. Essentially, if n = 2, an h1 becomes an h3. I didn’t find a solution within the current implementation. My initial idea was to use the resolver option with [BlockType.HEADING] (node) => {}. This would work, but I’d need to write my own headingResolver.

Since a developer might not think of all possible cases like the authors/contributors with resolving headings, I ended up copying the defaultRenderFn and headingResolver from the richtext class to modify the function with the behavior I needed.

My idea for the PR would be to export the standard resolvers with an additional options parameter to override rules. This could result in something like:

const {render, headingResolver} = richTextResolver({
  resolvers: {
    [BlockTypes.HEADING]: (node) => headingResolver(node, {level: node.level + 2})
  }
})

@alvarosabu alvarosabu added pending-triage [Issue] Ticket is pending to be prioritised feature [Issue] New feature or request labels Oct 1, 2024
@alvarosabu alvarosabu added p2-nice-to-have [Priority] Lower priority, beneficial enhancements that are not urgent. and removed pending-triage [Issue] Ticket is pending to be prioritised labels Oct 18, 2024
@alvarosabu
Copy link
Contributor

Hi @apfelbox @markus-gx thanks for the detailed feedback.

I think we can improve the DX further with some utilities like you mentioned, let's discuss which approach would be better with some context:

On the current version 2, none of the resolvers are exposed. All of the resolvers resolve internally to this function renderFn which by default is this for vanilla

richtext/src/richtext.ts

Lines 15 to 23 in f14a87d

function defaultRenderFn<T = string | null>(tag: string, attrs: Record<string, any> = {}, children: T): T {
const attrsString = attrsToString(attrs);
const tagString = attrsString ? `${tag} ${attrsString}` : tag;
if (SELF_CLOSING_TAGS.includes(tag)) {
return `<${tagString} />` as unknown as T;
}
return `<${tagString}>${Array.isArray(children) ? children.join('') : children || ''}</${tag}>` as unknown as T;
}

This function can be override by framework specific render functions.

// Vue
const options: StoryblokRichTextOptions<VNode> = {
  renderFn: h,
};

So if I understand the DX improvement would be to have an exposed wrapper for the renderFn so the user doesn't need to return the template string in the case of vanilla javascript, nor the h or React.createElementright?

@alvarosabu alvarosabu self-assigned this Oct 18, 2024
@alvarosabu alvarosabu added the pending-author [Issue] Awaiting further information or action from the issue author label Oct 28, 2024
@apfelbox
Copy link
Author

Hi @alvarosabu,

yes, that would work.
For my use case, I mainly want to set some attributes and just do the default rendering. So any way (exposing the main renderer / passing the renderer as argument / etc...) would help here.

One thing to keep in mind however is that it would be great, when I overwrite the default renderFn, that this would just work with any other resolvers without change:

const options: StoryblokRichTextOptions<VNode> = {
    renderFn: myRenderFn,
    resolvers: {
        [BlockTypes.HEADING]: (node) => headingResolver(node, {level: node.level + 2})
                                     // ^^^^^^^^^^^^^^^ as changed the renderFn above, 
                                     // I now need to change all my resolvers to use the new default resolver
    },
};

That's why initially I suggested that you might want to pass the default resolver, so that this case automatically works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature [Issue] New feature or request p2-nice-to-have [Priority] Lower priority, beneficial enhancements that are not urgent. pending-author [Issue] Awaiting further information or action from the issue author
Projects
None yet
Development

No branches or pull requests

3 participants