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

Ship goldmark-attributes extension #7548

Closed
mestaritonttu opened this issue Aug 11, 2020 · 9 comments · Fixed by #8215
Closed

Ship goldmark-attributes extension #7548

mestaritonttu opened this issue Aug 11, 2020 · 9 comments · Fixed by #8215

Comments

@mestaritonttu
Copy link

This goldmark parser extension adds support for block attributes in markdown

https://github.com/mdigger/goldmark-attributes

Would it be OK to ship it or should it be reimplemented in a different way for some reason?

@bep
Copy link
Member

bep commented Aug 12, 2020

Would it be OK to ship it or should it be reimplemented in a different way for some reason?

There is also a third option, and since CommonMark has spent a decade discussing this and @yuin did not add this to his implementation, maybe that's the most sensible option.

@mirenbz
Copy link

mirenbz commented Sep 14, 2020

Is there any way to have this working somehow at the moment?

@bep
Copy link
Member

bep commented Nov 30, 2020

Trying to ping @yuin and get his opinion; I agree that it would be really useful to be able to add CSS classes to blockquotes etc. My take on this is that the CommonMark people will never agree on a spec for this, so if we look pass the "political side" of this issue; is there a technical reason why we would not want to add this?

{.epigraph}
> Why, you may take the most gallant sailor, the most intrepid airman or the
> most audacious soldier, put them at a table together – what do you get? The
> sum of their fears.

@jmooring
Copy link
Member

jmooring commented Nov 30, 2020

If this is added at some point, I hope it is disabled by default. Not because I don't like the idea (I do), but because separation of content and style seems like the right starting point for most projects.

[markup.goldmark.extensions]
  attributes = false

EDIT: I forgot that we already (by default) allow attributes on headings, controlled by:

[markup.goldmark.parser]
  attribute = true

which allows you to set the element id, CSS classes, and additional attributes (e.g., style, data-foo, etc.). And I'm not sure the default should be true. Additionally, would an attributes extension allow setting element id, CSS classes, and additional attributes, or would it be limited to CSS classes only?

@bep
Copy link
Member

bep commented Nov 30, 2020

. Additionally, would an attributes extension allow setting element id, CSS classes, and additional attributes, or would it be limited to CSS classes only?

The linked extension above is very low on tests/documentation, so it's hard to tell. But if we want to add this to Hugo, I suspect we would need to re-implement it, so ... up to us. But I also do agree that this would be an option -- default off.

@yuin
Copy link

yuin commented Nov 30, 2020

I agree with @bep 's opinion.
Reasons why I wont to add general attributes to the goldmark core:

  1. It can be implemented (mostly) as an extension.

    • It is hard to implement heading attributes as an extension, so I've implemented it in the core.
  2. There are many interpretations for attributes positions and I do not know which is the best.

    1. Attribute is in one line above an element:

      {.class}
      > foo
      > bar
      
    2. Attribute is in one line below an element:

      > foo
      > bar
      {.class}
      
      1. Special case: blockquote can be:
      > foo
      > bar
      > {.class}
      
      1. (There may be other special cases)
  3. Some elements do not have suitable syntax for attributes:

    1. Lists
    2. Table cells

@jmooring
Copy link
Member

jmooring commented Nov 30, 2020

If all three (element id, CSS classes, and additional attributes) were possible, I would vote to control them separately. Something like:

[markup.goldmark.extensions]
  attributes = true
  [markup.goldmark.extensions.attributes]
     elementIDs = false
     cssClasses = true
     additionalAttributes = false

The additionalAttributes should be limited an "allow list". And at some point someone will ask for an "allow list" for cssClasses as well.

This is one of those features that has a high risk of scope creep.

@bep
Copy link
Member

bep commented Nov 30, 2020

OK, reading the Goldmark code now, I notice the new parser.WithAttribute option that is currently only available for titles (ref. discussion above); that does support any attributes and I assume it is the same parser that is used in the extension above.

bep added a commit to bep/hugo that referenced this issue Feb 8, 2021
E.g.:

```
> foo
> bar
{.myclass}
```

There are some current limitations: For tables you can currently only apply it to the full table, and for lists the ul/ol-nodes only, e.g.:

```
* Fruit
  * Apple
  * Orange
  * Banana
  {.fruits}
* Dairy
  * Milk
  * Cheese
  {.dairies}
{.list}
```

Fixes gohugoio#7548
bep added a commit to bep/hugo that referenced this issue Feb 8, 2021
E.g.:

```
> foo
> bar
{.myclass}
```

There are some current limitations: For tables you can currently only apply it to the full table, and for lists the ul/ol-nodes only, e.g.:

```
* Fruit
  * Apple
  * Orange
  * Banana
  {.fruits}
* Dairy
  * Milk
  * Cheese
  {.dairies}
{.list}
```

Fixes gohugoio#7548
@bep bep closed this as completed in #8215 Feb 8, 2021
bep added a commit that referenced this issue Feb 8, 2021
E.g.:

```
> foo
> bar
{.myclass}
```

There are some current limitations: For tables you can currently only apply it to the full table, and for lists the ul/ol-nodes only, e.g.:

```
* Fruit
  * Apple
  * Orange
  * Banana
  {.fruits}
* Dairy
  * Milk
  * Cheese
  {.dairies}
{.list}
```

Fixes #7548
@bep bep added Enhancement and removed Proposal labels Feb 18, 2021
@bep bep added this to the v0.81 milestone Feb 18, 2021
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants