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

Allow setting attributes on <ol> and <ul> tags with attr_list extension #1252

Closed
wants to merge 6 commits into from

Conversation

paulmelis
Copy link

@paulmelis paulmelis commented May 5, 2022

This adds a way for the attr_list extension to lift attribute values set on list items to their parent list element (i.e. <li> attribute to <ul> or <ol>). This allows list-wide attributes, such as list start number or class to be set by setting them on list items with the appropriate syntax.

Contains additions to the docs describing it, as well extra test input (plus all existing unit tests still pass)

Should be able to solve #227, #668 and many of the other requests for a similar feature, including my own quest to get this feature.

paulmelis added 2 commits May 5, 2022 22:55
values set on list items to their parent list element. This allows
list-wide attributes, such as start number or class to be set.
@facelessuser
Copy link
Collaborator

I'm not commenting on whether this feature should or should not be included, but how would you specify both a class on an <li> and on <ol> or <ul> if you must do it on the same line?

@paulmelis
Copy link
Author

I'm not commenting on whether this feature should or should not be included, but how would you specify both a class on an <li> and on <ol> or <ul> if you must do it on the same line?

As mentioned in the doc update this isn't possible with this patch. I initially started out with a parent: prefix (e.g. parent:attr=value) to be able to more precisely specify which attributes should be lifted to the parent, but decided against it as it needed quite more code changes. Plus I wonder how many times you would really want to be able to set attributes on both list items and the list itself. For the latter you only need to use an attribute on a single list item, while all the other list items can still have their own attributes.

@paulmelis
Copy link
Author

An alternative could be to have a single ^ as a marker in the attribute list to indicate that all attributes following it should be set on the parent. That would allow both list and table attributes with minimal code changes.

as a marker to indicate all attributes following it should
be lifted to the parent. This allows one to set attributes
on a list item, while also setting attributes on the parent.
@paulmelis
Copy link
Author

I updated the pull request to use ^ as a marker to signal when attributes should be lifted. Solves the issue of not being to define both list element attributes and parent attributes in the same attribute definition

@waylan
Copy link
Member

waylan commented May 6, 2022

As I have stated on every request we have received for this, we will not be implementing this feature. Of course, you are free to provide this functionality in a third party extension.

@waylan waylan closed this May 6, 2022
@paulmelis
Copy link
Author

As I have stated on every request we have received for this, we will not be implementing this feature. Of course, you are free to provide this functionality in a third party extension.

I have seen a number of similar comments about not wanting to implement it, but I'm really struggling to understand why? This feature is currently not available, yet is obviously valuable to many, given all the requests over the many years it has shown up as an issue here. The way I implement it here does not seem to interfere with any existing feature, as far as I can tell. And if the special ^ marker isn't used at all then there's no difference in behaviour, so it should be fully backwards compatible. It adds value, not only to me, yet it is coldly being rejected for unclear reasons other than "we will not be implementing this feature".

Please help me understand, because this all does not make any sense...

@waylan
Copy link
Member

waylan commented May 6, 2022

This has been discussed and explained many times over. You even linked to some of those discussions. In short, I am not interested in supporting a syntax that is does not make sense to me. It should be no surprise that it was rejected. Of course, you are free to disagree with me on this. If this is important to you, then you can provide and support your own third-party extension which provides the feature.

@Andre601
Copy link

Andre601 commented May 6, 2022

In short, I am not interested in supporting a syntax that is does not make sense to me.

From me as an outside-viewer I can see this response as essentially "I don't like this, so I don't want it" response.
You put your own bias over something that the majority of people may like to have for once. I saw this problem in MkDocs and see it here again. You seem to have a general issue with accepting things that you don't like, but that the community could benefit from.

@waylan
Copy link
Member

waylan commented May 6, 2022

I don't accept things that I'm not willing to support long term. Long after the person who proposes it is gone, I'm left defending it and fixing issues with it. That's (one of the reasons) why we provide a public extension API. Anyone can provide their own implementation which works how they want. Then they can support it without any additional burden on me. I am not limiting anyone at all except for myself.

@Python-Markdown Python-Markdown locked as too heated and limited conversation to collaborators May 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants