-
Notifications
You must be signed in to change notification settings - Fork 0
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
#198 - Markdown specification (part 1 of 2) #209
Conversation
tests/test_files/html/images.html
Outdated
<h2>Inline images</h2> | ||
<h2>Reference images</h2> | ||
<p>The reference is immediately after the image.</p> | ||
<p>The reference is not immediately after the iamge.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in image
tests/test_files/markdown/images.md
Outdated
text before ![alt text][image4] text after | ||
[image4]: https://www.example.com/test.png "title text" | ||
|
||
The reference is not immediately after the iamge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in image
|
||
The reference is immediately after the image. | ||
|
||
The reference is not immediately after the iamge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo in image
tests/test_files/html/lists.html
Outdated
<li>item F</li> | ||
</ul> | ||
</li> | ||
<li>item 3<ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be item 4
@@ -0,0 +1,4 @@ | |||
<p>Colons can be used to align columns.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want text in here indicating that tables will be stripped from html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here. I documented that in the index file. The input markdown reflected here is copied directly from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just mean to write some text in here like you did for the images.html that says the table is going to be stripped. I found it confusing to read the text that is here and to not see any tables, but I will defer to your judgement.
@@ -0,0 +1,5 @@ | |||
Colons can be used to align columns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want text in here indicating that tables will be stripped from plain text?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not here. I documented that in the index file. The input markdown reflected here is copied directly from here.
6. item 3 | ||
6. item 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean for these to both be 6? they aren't in the html above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's intentional. Markdown just requires digits to denote an ordered list. They don't need to be unique or in order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you note that somewhere so that someone doesn't come along later and 'fix' it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple additional typos
tests/test_files/index.md
Outdated
|
||
## Unsupported markdown | ||
|
||
VA Notify markdown is not gauranteed to support: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guaranteed
tests/test_files/index.md
Outdated
|
||
## Supported markdown | ||
|
||
VA Notify markdown support mostly aligns with [Github markdown](https://github.com/adam-p/markdown-here/wiki/Markdown-Cheatsheet). Some addtionally supported markdown includes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
additionally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approved with comments. i fear that anyone working any part of this who is not you will not be able to get up to speed on what is happening here.
Description
These changes add test markdown templates that should serve as a comprehensive specification for how the Notify app should convert markdown entered into Portal into HTML or plain text.
The expected HTML intentionally does not include any inline styling, which departs from the current implementation. The intent is that a future implementation will apply all CSS through Jinja templates rather than using custom Mistune to add inline styles.
These changes do not include any markdown templates with placeholders. They will be added as necessary for #199.
These changes do not include any unit tests or implementation.
issue #198
How Has This Been Tested?
Checklist