-
Notifications
You must be signed in to change notification settings - Fork 181
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
Syntax proposal for new html attribute plugin #264
Comments
I agree we should aim to be consistent with these syntaxes. @vincerubinetti is there any other relevant context like the jQuery |
<!-- explanatory text $colspan="2" more explanatory text $style="color: red" even more explanatory text --> I like the proposal, but I think we should require that comments that wish to set attributions begin with As far as the attribute indicator of Sorry it took me a while to get around to commenting here. Looking forward to releasing #251. |
I should also say that this would make me comfortable enabling the plugin in the default rootstock build, since I think it's highly unlikely someone would trigger this by mistake. |
@dhimmel so your modified proposal is <!-- setattrs explanatory text $colspan="2" more explanatory text $style="color: red" even more explanatory text --> I have no objections to that. |
I think The html attribute names are pretty specific. No one's going to type colspan without knowing that it's an html attribute. And even if someone does accidentally trigger it, it will either change the look of the html a bit (in which case it's pretty easy to realize what's going on), or just do nothing if it's an invalid html attribute. In fact, even though there's a lot of attributes, almost all of them only apply to certain types of elements. So if you were to look through all the permutations of attributes and elements, the vast majority of them do nothing or nothing significant. Also with regard to processing. Parsing strings in javascript is very fast and even with thousands of comments the time would be trivial (especially compared to the other processing being done by the other plugins on a manuscript of that size). |
Here is a test with 891 comments: median times on my local machine, in chrome: The same test but with half of the comments starting with median times on my local machine, in chrome: |
Yes. @vincerubinetti I agree the speed and reliability benefits of <!-- disable setattrs $colspan="2" $style="color: red" --> would not set attributes. Without setattrs, the user would have to delete every
The test is pretty cool and helpful. The type of comments where setattrs would show the greatest benefits will likely be very long comments. For example, perhaps someone will insert 1000 lines of commented source code to create a figure. Not that I suggest users do this, but that I think its possible. In this case, short-circuiting the evaluation of that comment could have more major time savings. |
If you can provide me with some cases to test that would be helpful. But even with the most massive papers, the time savings is going to be negligible compared to the work the other plugins will also have to do, and I think it's an anti-pattern to worry about that. If you want to be able to comment out a whole comment at once, I think that's valid. Though I don't imagine people would need to be commenting and uncommenting these types of comments very often. The use case for this plugin is already very edge case... mostly just for colspan. |
Also maybe |
I imagine including source code for figures via comments would be one thing users will do. You could try putting this R-code for a figure from a recent publication. This file is 178 lines. In the worst case, we could imagine someone doing something like (on the order of 500 line comments) for all of their figures.
|
Would you be able to put together a test dummy manuscript with the absolute worst case you'd ever expect (within reason) in regards to comments.
No, id is just |
And Pandoc's
Okay here's a manuscript with several 100+ line comments of code (python, R, shell, matlab, javascript). It's based off of the meta-review HTML: manuscript-with-comments.html.txt. It's not absolute worst case... some users may have bigger comments, but it should be enough for us to get meaningful performance estimates. Would also be good to see if attributes are extracted from any of these comments by the plugin. |
Yeah, Pandoc's format is clearly modeled after CSS, where Okay, I tested the plugin on the sample manuscript you gave me. There were 0 false positives (no comments were parsed at all), and it takes less than a millisecond on average. Tested it on mobile, and there is no perceptible lag from the plugin on my iPhone SE (fairly modern), or my HTC One (fairly old). |
Some further testing: Parsing the comment doesn't even take most of the time. Most of the time is taken by actually setting the attribute in the DOM. DOM manipulations are very slow compared to everything else, and as I said before, string parsing and manipulation in javascript is actually very fast. At this point I would definitely say that testing or optimizing for speed is an anti pattern, especially considering we haven't done this for any of the other plugins. The discussion should be confined to what will be easiest for the user to remember, and what will cause the least false positives. And as I've stated, I really think false positives are incredibly unlikely (and very very low consequences even if they do happen), so that just leaves which is easier to remember and/or use: The desire to be able to uncomment/comment a whole line of the plugin at once is something to be considered, but I really don't think people are going to need to switch these things on and off. The use case is already so edge. In fact, it's really unlikely people will set more than one attribute in a comment, or even use the plugin at all. |
Okay, given that you didn't have any false positives on the comment-heavy example manuscript, let's proceed without Also I am okay enabling the plugin by default since it seems unlikely to cause much harm even if it does falsely trigger.
I don't have a strong opinion. @vincerubinetti your call. Is #251 ready for review / merging? |
Not ready yet. I'll have it by the end of today. |
This build is based on 0dc5ea8. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/127061386 https://travis-ci.com/manubot/rootstock/jobs/234117445 The full commit message that triggered this build is copied below: Plugin to set HTML attributes via comments Merges #251 Closes #264 Refs #240 Uses javascript to apply attributes extracted from HTML comments.
This build is based on 0dc5ea8. This commit was created by the following Travis CI build and job: https://travis-ci.com/manubot/rootstock/builds/127061386 https://travis-ci.com/manubot/rootstock/jobs/234117445 The full commit message that triggered this build is copied below: Plugin to set HTML attributes via comments Merges #251 Closes #264 Refs #240 Uses javascript to apply attributes extracted from HTML comments.
This build is based on 0dc5ea8. This commit was created by the following Travis CI build and job: https://travis-ci.com/RHagenson/biological-messaging/builds/128096845 https://travis-ci.com/RHagenson/biological-messaging/jobs/236645304 The full commit message that triggered this build is copied below: Plugin to set HTML attributes via comments Merges manubot/rootstock#251 Closes manubot/rootstock#264 Refs manubot/rootstock#240 Uses javascript to apply attributes extracted from HTML comments.
This build is based on 0dc5ea8. This commit was created by the following Travis CI build and job: https://travis-ci.com/RHagenson/biological-messaging/builds/128096845 https://travis-ci.com/RHagenson/biological-messaging/jobs/236645304 The full commit message that triggered this build is copied below: Plugin to set HTML attributes via comments Merges manubot/rootstock#251 Closes manubot/rootstock#264 Refs manubot/rootstock#240 Uses javascript to apply attributes extracted from HTML comments.
Merges manubot/rootstock#251 Closes manubot/rootstock#264 Refs manubot/rootstock#240 Uses javascript to apply attributes extracted from HTML comments.
Merges manubot/rootstock#251 Closes manubot/rootstock#264 Refs manubot/rootstock#240 Uses javascript to apply attributes extracted from HTML comments.
We are a new plugin in #251 that would allow users to attach arbitrary HTML attributes to almost any element in markdown.
Briefly, it works like this: The user puts in an HTML comment like
<!-- comment -->
in their markdown next to the desired element. The comment will contain some kind of text indicating what HTML attribute to set and to what value. Then pandoc will pass the comment through untouched to the outputted.html
. Then the plugin, when loading the manuscript, will scan the.html
document for comments matching the agreed-upon syntax, parse them, and attach the attributes.This issue is to discuss what the syntax inside the HTML comment should look like.
Here are some of the goals:
Here is my proposal
<!-- explanatory text $colspan="2" more explanatory text $style="color: red" even more explanatory text -->
The plugin will look for a
$
, followed by a continuous (no whitespace) word, followed by a=
, followed by a pair of"
.With this, you can take out the
$
to add/remove attributes fairly easily. I chose$
because jQuery uses it, and many people already are used to seeing it in HTML and javascript. We could also use#
.The text was updated successfully, but these errors were encountered: