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

Improve image and link nodes #156

Open
3 tasks
mfix-stripe opened this issue Aug 1, 2022 · 13 comments
Open
3 tasks

Improve image and link nodes #156

mfix-stripe opened this issue Aug 1, 2022 · 13 comments
Assignees
Labels
enhancement New feature or request

Comments

@mfix-stripe
Copy link
Contributor

mfix-stripe commented Aug 1, 2022

  • Update image syntax so that if it is on a line by itself, it is treated as a block, otherwise treat it as an inline element.
    • The default schema would render block-level images in a figure instead of a p, but this behavior would be possible to override in a custom node definition’s transform function
  • Update image syntax to allow passing additional attributes
    • E.g. ![alt](/href width=10 height=10 foo={arbitrary: "markdoc stuff"})
  • Test that the image/link syntax works with GitHub flavored Markdown, or we have a story for that.
@nvanexan
Copy link
Contributor

Hey all 👋 - quick question re additional attributes. Does this mean Markdoc will be departing from the commonmark spec for images at the parsing phase? E.g. https://spec.commonmark.org/0.30/#example-571

@mfix-stripe
Copy link
Contributor Author

Hey all 👋 - quick question re additional attributes. Does this mean Markdoc will be departing from the commonmark spec for images at the parsing phase? E.g. https://spec.commonmark.org/0.30/#example-571

@nvanexan potentially. Though depending on implementation specifics, we could make it a strict syntactical extension. Do you have concerns with going this direction?

@nvanexan
Copy link
Contributor

Hey all 👋 - quick question re additional attributes. Does this mean Markdoc will be departing from the commonmark spec for images at the parsing phase? E.g. https://spec.commonmark.org/0.30/#example-571

@nvanexan potentially. Though depending on implementation specifics, we could make it a strict syntactical extension. Do you have concerns with going this direction?

@mfix-stripe no, not really. I can see this being valuable. I had some hesitation about portability and compatibility when I saw the example above. For example, if I use those additional attributes in an .md file, does that make my images un-renderable in other contexts (e.g. GitHub, iAWriter, etc.)? And if I have files already implementing commonmark image patterns with a string for a title/caption ![foo](/url "title"), will that continue to work? But as long as the custom attributes are documented somewhere, and their implementation doesn’t break existing commonmark image patterns, I’d welcome it for sure. 😊

@mfix-stripe
Copy link
Contributor Author

@nvanexan if you are using and Markdoc specific syntax (e.g. annotations), then those will not show up correctly in GitHub or iAWriter, until they support Markdoc syntax. If you are not, and you just use standard Markdown (e.g. ![foo](/url "title")), then it will still work in both all the contexts.

@samuelstroschein
Copy link

Are images nested in links part of this issue?

I just ran into the issue of using images with links. The following markdown throws { "id": "child-invalid", "level": "warning", "message": "Can't nest 'image' in 'link'" } but is valid on GitHub and a CommonMark reference implementation https://spec.commonmark.org/dingus/.

[![Open in Gitpod](https://gitpod.io/button/open-in-gitpod.svg)](https://gitpod.io/#https://github.com/inlang/inlang)

@paulrudy
Copy link

I see this is on the roadmap as "ready", and I'm curious whether there's a general ETA for this feature at this point?

@jaanli
Copy link

jaanli commented May 19, 2023

Also following up on this: is the Image component (https://nextjs.org/docs/pages/api-reference/components/image) not supported?

(Trying to fix the images here: onefact.org/five-boro-bike-tour/jaan)

@mfix-stripe
Copy link
Contributor Author

Also following up on this: is the Image component (https://nextjs.org/docs/pages/api-reference/components/image) not supported?

@jaanli you can use Next.js's Image component as the backing component for your image node/tag, yes. But it needs to be added to your Markdoc schema. The problem w/ using it as an image node right now is that Image requires a width and height be passed as props, and there isn't a good way to pass those to the node yet. You could calculate the image size at build and pass them as props via the image node's transform function.

@rpaul-stripe
Copy link
Contributor

rpaul-stripe commented Jun 7, 2023

This has come up a few times recently, so I want to revisit it and get some opinions about syntax. There are a bunch of different approaches to this problem in different Markdown engines and environments. There are three approaches that I'm currently considering:

  1. We could modify the parser to use custom syntax, basically extending it so that we can add arbitrary Markdoc parameters to the end of a link or image: ![foo](foo.png width=200 height=300 #foo)

  2. Or we could make it so that an annotation inside of the square brackets binds to the image/link: ![foo {% width=200 height=300 #foo %}](foo.png)

    This example is currently syntactically valid, but it puts the attributes on the enclosing paragraph instead of on the image. We could add an option that you could enable in the parser to make it put the attributes on the image instead of the paragraph when it is enclosed inside of the square brackets.

  3. The third option is to put the annotation immediately after the link/image with no spaces between them: ![foo](foo.png){% width=200 height=300 #foo %}

    The challenge with this approach is that it's ambiguous with annotations on the surrounding block-level element. We would have to treat it as an annotation on the image and link only when it immediately follows it with no spaces. That would mean that e.g. [foo](/foo){% #x %} is a link with an attribute whereas [foo](/foo) {% #x %} is a link with an attribute on the enclosing paragraph.

I'd like to get some feedback on which of these options the community likes better. I personally am leaning towards the second one. I think the first one is a little cleaner from a readability standpoint, but the second one is a lot more consistent with Markdoc's existing design and it avoids having to introduce new syntax or parser modifications that may be difficult for other implementations to support.

@4141done
Copy link

4141done commented Jun 7, 2023

Here's my two cents on the syntax suggestions given by @rpaul-stripe. I think context is valuable here - so I should clarify that I'm aiming enable open source contributors to our documentation as well as internal folks who have no knowledge of markdoc.

I like suggestion (3) in theory the most, but in light of the technical challenges and also potential issues with the lack of a space becoming a critical piece of syntax which AFAIK isn't very markdown-y in inline elements.

In light of that I think (2) is a good suggestion because:

  • It won't trip up people or automatic generators that are used to the title attribute being specified after the href as in (1)
  • It maintains the familiar {%%} syntax which signals "this is not part of standard markdown" which will I think lower cognitive load for those we're asking to use some nonstandard elements in their authoring.

Thank you for your work on this 🐳

@mfix-stripe mfix-stripe changed the title Improve image node Improve image and link nodes Jun 7, 2023
@jaanli
Copy link

jaanli commented Jun 29, 2023

Do you know whether it's possible to have Markdoc support this standard Next.js feature of the Image component?

From the docs - https://nextjs.org/docs/app/building-your-application/optimizing/images -

Next.js will automatically determine the width and height of your image based on the imported file.

Attempt at a minimal reproducible example:

jaan.li/birth-support

Commit here adding a Markdoc component given your advice:

jaanli/jaan.li@06ab272

It seems malformed and despite the documentation claiming that the image size is read automatically, I haven't been able to figure out how to enable this feature.

Appreciate any help on this -- thank you @rpaul-stripe & @mfix-stripe! I'm getting a little closer but still struggling & need to complete my doula certification in the upcoming months. It's a huge help having @markdoc to rely on as I open source several birth support pages.

@mfix-stripe
Copy link
Contributor Author

@jaanli you should be able to use the Next.js Image component by adding an image tag in your own schema.

// ./markdoc/tags.js
import Image from 'next/image'

export.image = {
  render: Image, 
  // ...
}

However, I am going to add a default image tag to @markdoc/next.js here: markdoc/next.js#27. Previously there were complications where image components required a loader, which isn't easy to pass in Markdoc. But with next@13 this is no longer the case.

@RomanHotsiy
Copy link
Collaborator

I'd like to get some feedback on which of these options the community likes better.

I would vote on (2) as well. Mainly because it doesn't break the content if used with another markdown parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 🔖 Ready for developement
Development

No branches or pull requests

8 participants