Admonition: add the possibility to specify additional CSS classes names#580
Admonition: add the possibility to specify additional CSS classes names#580WhiteWinterWolf wants to merge 6 commits intoPython-Markdown:masterfrom
Conversation
Additional CSS classes names can be appended to the admonition name using dots as a separator.
The following markdown:
!!!note.floatright
This is a floating note.
Generates the following HTML code:
<div class="admonition note floatright">
<p class="admonition-title">Note</p>
<p>This is a floating note.</p>
</div>
Updated the regex to include the fix from the issue Python-Markdown#550.
|
From what I understand, if this were to be accepted, it most likely wouldn't be accepted until 3.0. With that said, why use |
|
@facelessuser Yes, since the quotes are mandatory even for single word titles then space should be technically fine (as would do comma, or pretty any other punctuation except the minus character). I saw the space as an argument separator, where the first argument defines the CSS classes and the second one defines the title. If it seems more natural to have several space-separated arguments as classes except the latest one which gets a special meaning when surrounded by quotes and becomes a title instead of a CSS class, why not. This still seem a little error prone to me, but this is just my current feeling and I may be wrong. Thinking in terms of arguments, another possibility would be to just require the quotes on multiple-words arguments both for the classes and the title, making these syntaxes legal: This may also feel natural and should be easy to implement (the class name is already mandatory and its value cannot contain quotes). |
I don't personally see a problem with spaces. The title is very specific, and with the syntax being strict as it is in this regard, I don't see a problem with just spaces. But sure we could technically use any delimiter that doesn't make syntax evaluation difficult. I think
You're probably going to have a harder time getting a rework of the syntax vs an expanding of the syntax approved. I don't think quoting the classes is a good idea as it will just make things more confusing. |
|
@facelessuser : I have updated the patch with your suggestion to use spaces as class names separators. Travis now fails but I have the impression that this is unrelated to my modification (checkspelling now fails on code_hilite.html). |
|
I imagine Travis occasionally upgrades their Ubuntu and that sometimes upgrades aspell version being used. I would just add Most likely, if @waylan is okay with this moving forward, it will probably be required that you put in some unit tests to verify the new format. Also, I think currently admonitions takes the first class and makes that the title if no title is given? I think we'd need a confirmation from @waylan whether that is ideal, or if all provided classes should be taken into consideration. I think in general, the first seems okay to me. It is doubtful anyone would be spelling out a reasonable title with CSS classes :). |
|
Thinking about this more, you can probably ignore the spelling issue. I don't know if/when this is getting merged, so I imagine the spelling will get fixed well before this gets merged. |
|
Initially I was going to say that I have no interest in this. But then I read through the rest of the discussion. Space separated classes with a quote wrapped title seem fine. And yes, if no title is provided, the first class should become the title. No backward compatibility issues for existing documents there. That said, I'm not merging this until version 3.0. |
|
@facelessuser @waylan: Thanks for your feedback, I've updated the unit tests and documentation to match the new feature. I'm fine with an inclusion in 3.0. |
markdown/extensions/admonition.py
Outdated
|
|
||
| def get_class_and_title(self, match): | ||
| klass, title = match.group(1).lower(), match.group(2) | ||
| klass = re.sub(' +', ' ', klass) |
There was a problem hiding this comment.
In general, I prefer (when possible) to pre-compile regex. This would be one that can easily be either compiled once per file or at least once per class instantiation (either is probably fine). This isn't really a big issue, but every little bit helps. Other than that, I have no complaints with anything else.
There was a problem hiding this comment.
Yes all reflex should be compiled as soon as possible.
Codecov Report
@@ Coverage Diff @@
## master #580 +/- ##
=========================================
Coverage ? 93.73%
=========================================
Files ? 29
Lines ? 2859
Branches ? 0
=========================================
Hits ? 2680
Misses ? 179
Partials ? 0
Continue to review full report at Codecov.
|
markdown/extensions/admonition.py
Outdated
|
|
||
| def makeExtension(*args, **kwargs): | ||
| return AdmonitionExtension(*args, **kwargs) | ||
|
|
There was a problem hiding this comment.
You broke linting with this extra line. There should only be one newline at the end of files, not two.
|
Thanks @WhiteWinterWolf, that covers all my complaints. |
|
Hi @facelessuser, Sorry for the inconvenience, Nevertheless, I do not see how this would break Python. A quick check gives me leads to ESLint (for Java) which in fact changed their own rules from requiring exactly one linefeed characters (the issue we have here) to having at least one linefeed character, allowing more than one. The actual issue that needs to be detected is indeed when there is no linefeed at the end of the file. Nevertheless, the culprit character has been removed now (and yes, I know: "vi is the evil" ;) !). |
This is not a question of whether it will break Python. A lot of the checks done by flake8 are stylistic. It is perfectly acceptable for a project to enforce style guides it prefers whether or not they "break" compilation or parsing. You can always run linting locally, before pushing, to avoid unexpected lint issues in the review. |
|
Interesting side-effect of making this change is that I noticed for the first time that we have an incorrectly formatted admonition (see #583). The 'spellchecking' test is failing because of the GitHub username "richeland" not being in the spelling dictionary. I was curios why that didn't fail any other tests and realized that the word only appears within the incorrectly formatted admonition. That admonition's title ("see also") was missing quotes, so the class was "see" with the body of the admonition being "also". The actual body was being rendered as a codeblock. And as the spellchecker skips codeblocks, it was not catching "richeland". With this change, both "see" and "also" were being assigned as classes and the body was being properly identified. Therefore, for the first time, the body of the admonition was being checked by the 'spellchecking' test. |
|
Ah, that makes sense. I was also wondering why it didn't fail on master and other pulls since this one. I had assumed there was an aspell update (as that has happened to me on other repos). But it makes sense as we are now handling the previously erroneous extra classes. |
|
As this was originally created on the master branch, I couldn't easily add a remote to my local repo to clone it and rebase. Therefore, I've opened a new PR at #685 and this is being closed in favor of that. |
3 participants
Hi,
I read about the feature freeze in the other pull requests, so feel free to tell me if there is a better location to propose this patch.
I need to be able to add additional classes to admonition, typically to make some box floating on the right while keeping the other ones in full width. I don't think there is a way to make the Admonition extension to work with the Attribute List to achieve this, so I updated Admonition to add this feature.
Additional CSS classes names can now be appended to the admonition name using dots as a separator.
The following markdown:
Now generates the following HTML code:
Dots are currently invalid in admonition names and break the admonition box, so I think it is safe to assume that this change should not cause any compatibility issue as nobody should be already using such dotted syntax or expect any particular behavior, and dots are already used as class names separator in CSS files so this syntax seems natural to me.