-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(comments): add Markdown support #54685
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
Conversation
|
Labels ? Milestone ? |
|
I fear a bit that this break accessibility of the comments because of headings. # heading 1
some testinto the markdown than this will be rendered as |
Here's the trick: we don't 🤷🏽 |
@skjnldsv it is not scheduled for Nextcloud 32 specifically, since feature freeze has passed |
Ensure that NcRichText satisfies a11y requirements, as I'm not sure it was a part of BITV certification (Talk app was not). Then it should be good to go |
|
@Antreesy and do you know who I can ask this to? |
I'd assume, Ferdinand, Grigorii or Julia might help |
|
@JuliaKirschenheuter @ShGKme @susnux can some of you review this PR please? There are concerns regarding the accessibility |
|
ping @JuliaKirschenheuter |
|
Checking the possibilities |
|
The only solution I see: while keeping the original style, level down headings inside NcRichText:
I'd also consider making the style of the heading smaller, so people don't use |
|
This also breaks comments of users using only |
We do similar in apps/settings/src/components/Markdown.vue |
|
I wouldn't limit the users from using Instead, I'd render them as low-level headings, and "disrespect" using In this case users by default use valid internal and short structure. |
Thats what it is doing. |
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
81eeac6 to
6d4da35
Compare
|
Hey there. This has been rebased on master, all good with nc/vue 9.3.0 and the max title level. |
Antreesy
left a comment
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.
So far looks fine. If some markdown styles are to be adjusted, it's better to do it upstream and keep it simple here
There's also use-extended-markdown prop that add some more features, as markdown tables or code syntax highlight, but I don't think it's worth it (plus it also increases the bundle size)
| :deep(img) { | ||
| max-width: 100%; | ||
| height: auto; | ||
| } |
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.
Maybe we need to add it to upstream. Or forbid images...
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.
Soudns good and should than be a follow up 👍
But in async chunk |
@Antreesy, I'd appreciate those features. They're mostly why I wanted this functionality. |
Summary
Enable markdown support for comment content block (NcRichText). Increase max comment block height limit, add scroll.
Checklist