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

⏩ Extend renderers to allow for kind/subtypes #460

Merged
merged 2 commits into from
Aug 31, 2024
Merged

Conversation

rowanc1
Copy link
Member

@rowanc1 rowanc1 commented Aug 31, 2024

This allows for multiple renderers to be attached to a single node, based on a match of the kind.

For example you can now add a specific renderer for a link[kind=github]. This allows for inheriting these renderers and then extending or overriding them independently.

Copy link

changeset-bot bot commented Aug 31, 2024

🦋 Changeset detected

Latest commit: 5fbaeb0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 12 packages
Name Type
myst-to-react Patch
@myst-theme/providers Patch
@myst-theme/jupyter Patch
@myst-theme/site Patch
@myst-theme/frontmatter Patch
@myst-theme/diagrams Patch
@myst-theme/styles Patch
@myst-theme/common Patch
@myst-theme/icons Patch
@myst-theme/book Patch
@myst-theme/article Patch
myst-demo Patch

Not sure what this means? Click here to learn what changesets are.

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

@rowanc1 rowanc1 marked this pull request as ready for review August 31, 2024 02:50
'link[protocol=rrid]': RRIDLinkRenderer,
'link[protocol=ror]': RORLinkRenderer,
// Put the kinds last as they will match first in the future
'link[kind=github]': GithubLinkRenderer,
Copy link
Member Author

Choose a reason for hiding this comment

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

Prepares for:

jupyter-book/mystmd#1417

@rowanc1 rowanc1 merged commit ed5a7f6 into main Aug 31, 2024
2 checks passed
@rowanc1 rowanc1 deleted the feat/renderers branch August 31, 2024 03:20
Copy link
Contributor

@fwkoch fwkoch left a comment

Choose a reason for hiding this comment

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

I think this makes sense. Now, instead of a simple type: renderer lookup, we have a more complex type: { renderers } structure where the basic renderer is under base and other renderers are under selector keys.

Keying off selector strings seems a little complicated (as opposed to, say, just kind or something) but it is quite flexible and powerful. Some of this logic is a little hard to parse without a bit more docstring-level documentation, though.

export function selectRenderer(renderers: NodeRenderersValidated, node: GenericNode) {
const componentRenderers = renderers[node.type] ?? renderers['DefaultComponent'];
const SpecificComponent = Object.entries(componentRenderers)
.reverse()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why reverse?

return validatedRenderers;
}

export function mergeRenderers(renderers: NodeRenderers[], validate: true): NodeRenderersValidated;
Copy link
Contributor

Choose a reason for hiding this comment

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

A few more docstrings - particularly for the functions in this file - would be helpful.

@@ -10,17 +12,25 @@ function DefaultComponent({ node }: { node: GenericNode }) {
);
}

export function selectRenderer(renderers: NodeRenderersValidated, node: GenericNode) {
const componentRenderers = renderers[node.type] ?? renderers['DefaultComponent'];
const SpecificComponent = Object.entries(componentRenderers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const SpecificComponent = Object.entries(componentRenderers)
const specificComponent = Object.entries(componentRenderers)

const LINK_RENDERERS = {
link,
const LINK_RENDERERS: NodeRenderers = {
link: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite see why we need this nested structure as opposed to:

{
  link: SimpleLink,
  'link[protocol=github]': GithubLinkRenderer,
  'link[protocol=wiki]': WikiLinkRenderer,
  ...
}

I guess the nesting lets us first narrow down based on type so we are not doing selector checks against all the renderers?

Copy link
Contributor

Choose a reason for hiding this comment

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

My next question is: would this work the same if we did something like:

{
  link: {
    base: SimpleLink,
    '[protocol=github]': GithubLinkRenderer,
    '[protocol=wiki]': WikiLinkRenderer,
    ...
  }
}

i.e. we don't need to repeat "link" since we are only dealing with link nodes at this point?

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