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

[FIX] Support complete markdown specification #7454

Open
wants to merge 22 commits into
base: develop
Choose a base branch
from

Conversation

gromain
Copy link

@gromain gromain commented Jul 9, 2017

@RocketChat/core

Closes RocketChat/feature-requests#642 Support full Markdown specification
Also solves:
#7240: Special characters escape with \ (_ ` etc...)
#6719 #6586: Markdown link with & sign is broken
#5218: Italicizing links breaks them
#4346: Support for markdown tables

Also, it may simplify work on #1593: support for better code highlight

This PR adds full Markdown support through markdown-it. It also supports sub- and super-scripts and checkboxes (via markdown-it plugins).
I believe some cleanup is needed in the CSS file(this one has been moved out of the base.css file into its own markdown.css. The class has been switched from .body to .markdown-body in message.html).

A couple of things are broken right now:

  • >>> to start a blockquote
  • Strikethrough with single ~ (works with ~~)
  • Bold with single * (works with ** or __)
    How do we deal with those style changes (bold and strikethrough and blockquote)?
  • Code highlighter could use a pass on the style used I think, some languages does not display well (python for example)

Screenshots

  • Tables
    Tables
  • Ordered and unordered lists
    lists
  • Checkboxes!
    checkboxes
  • Sub-scripts and super-scripts
    subsuperscripts
  • Typographic replacement
    typographic
  • Headings and horizontal rules
    headings
  • Code with highlighter
    codehighlight
  • Nested blockquotes
    blockquote

@gromain
Copy link
Author

gromain commented Jul 9, 2017

I'm not sure the build failure has anything to do with my changes.
The failure happens during a call to sideNav.getChannelFromSpotlight('rocket.cat').waitForVisible(5000); in 09-channel.js.

What can I do to fix that?

@gromain
Copy link
Author

gromain commented Jul 17, 2017

I pulled and merged the changes from Rocket.Chat/develop, I hope it will help with the build failure.

@gromain
Copy link
Author

gromain commented Jul 17, 2017

The build failure is now related to badwords management.
This PR add supports for horizontal rules via ___, --- or ***.
Bad words are replaced by *, thus creating a horizontal rule.
I will change the replacement to escape the stars.
We could also change the character to x.

@gromain
Copy link
Author

gromain commented Jul 17, 2017

Mmmmmm, for some reason, it worked on my local tests, but not when deployed.
Oh well, x it is then.

@gromain
Copy link
Author

gromain commented Aug 9, 2017

Hello @sampaiodiego @rodrigok and @MartinSchoeler,

Is there anything needed from my side?

@JSzaszvari
Copy link
Contributor

JSzaszvari commented Aug 9, 2017 via email

@gromain
Copy link
Author

gromain commented Aug 16, 2017

@JSzaszvari Agreed! :-D

@JSzaszvari
Copy link
Contributor

Keen to find out why this one has not been looked at? This would be a super helpful, especially the tables and lists

@geekgonecrazy @graywolf336 any chance you can let @gromain know what needs to be done if anything to move forward with this?

Thanks

@JSzaszvari
Copy link
Contributor

@engelgabriel ??

@Lemmmy
Copy link

Lemmmy commented Aug 24, 2017

Are you going to add support for nested blockquotes? E.g:

hello

world

foo

@Lemmmy
Copy link

Lemmmy commented Aug 24, 2017

Also - slack added a simple check to disable blockquoting with emoticons like >.> and >:( - would you be able to implement something similar?

@gromain
Copy link
Author

gromain commented Aug 25, 2017

@Lemmmy I forgot the nested blockquote support!
But yes, it's there:
blockquote

Regarding the emoticons, I'll have to have a look how to deal with this. It doesn't sound impossible though. It works for now if you escape the first > with \ though (but I agree, this is not practical at all!).
emoticone

@geekgonecrazy
Copy link
Contributor

@RocketChat/core can we try and get this lined up for 0.60.0 ? Better markdown support would be great to have

Copy link
Contributor

@lindoelio lindoelio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gromain! It seems a great work! I'm motivated to help merge this on next release. Can you update your PR with our current base code from develop branch?

…eded on the CSS and in the highlight code probably
@gromain
Copy link
Author

gromain commented Oct 9, 2017

I guess we can close this PR now.

To me it doesn't make sense since there is already work that was done in #7852 (and #6158).

It would be nice if the coordination on this project was more open and more communicative. I would have like to know when I asked in RocketChat/feature-requests#642 what were the thoughts of the community on the integration of Markdown.
I'm glad it's finally finding its way in Rocket.Chat, but in my humble opinion, the whole project would greatly benefit from a better stewardship and openness towards exterior contribution.
This experience only leaves me frustrated (and not because my changes don't make it through to master). The process is obscure at best. It would have been better if from the onset someone said "Hey, we are already considering pulling this PR, no need to sweat on it, but have a look if it can be improved". This would have been much better than just the screams of silence.

Anyway, I'll close this PR in a week if there is no further interest.

@JSzaszvari
Copy link
Contributor

JSzaszvari commented Oct 9, 2017

agreed. hoping that someone in @RocketChat/core has something to say considering this PR was opened a long time before the other markdown stuff was done and merged.

someone could have at least told the poor guy working on this one not to bother if there was plans that weren't made public.

instead multiple calls for a Comment went completely ignored..

@gromain
Copy link
Author

gromain commented Oct 30, 2017

Rocket.Chat is a great messaging app, but really, it would benefit greatly for more openness from the devs and a more open communication.

Closing this PR now.

@gromain gromain closed this Oct 30, 2017
@geekgonecrazy
Copy link
Contributor

Sorry guys. I'd also love to improve our markdown support. Being able to add support for additional markdown would be amazing.

I don't think another option has been chosen. We are constantly fighting to keep xss vulnerability from popping up. Anything that involves HTML in the message in any form runs a risk of being exploited.

Also we just finished releasing 0.59.0 and we are just now looking at PR's to merge into 0.60.0

With a lot of UI changes in 0.59.0 it took a lot of our time.

@rocketchat/core

@gromain
Copy link
Author

gromain commented Oct 30, 2017

@geekgonecrazy let me know if there is any interest and feedback, I will reopen it.

@rodrigok
Copy link
Member

@gromain Sorry about that, we was busy with our latest release and we delayed this review.

Please reopen it and I will start the review.

First question, did you change how bold and italic are handled?

@rodrigok rodrigok self-assigned this Oct 30, 2017
@geekgonecrazy geekgonecrazy reopened this Oct 30, 2017
@rodrigok rodrigok added this to the 0.61.0 milestone Dec 8, 2017
@rodrigok rodrigok modified the milestones: 0.61.0, 0.61.1 Jan 22, 2018
@rodrigok rodrigok modified the milestones: 0.61.1, 0.63.0 Feb 13, 2018
@DeanVanGreunen
Copy link

DeanVanGreunen commented Feb 22, 2018

Hey guys/girls, etc, etc.

having a small issue with the Chat app, desktop (just a wrapper app...) and web versions.

When typing the following:
Any Message which is then Wrapped in Code, StrikeThrough, Italics and bold as below real-world example/preview which is also marked as a multi-line.

Example Preview of Chat textbox

when sending/posting it in the chat, it will display as follows. (note: only name and date/time is marked out, due to company specs, hosted locally on internal servers, etc, etc..)

Same as preview as Example above

Both these images are of the exact same "data"/"text"

please explain what this is for and/or if it is meant to be avalible as a feature, is a bug, or if it is encryption of some sorts, what is this data and how do I undo it??? or disable this if it is a bug..

This problem can be repeated and will always reproduce the same "output/display data" if the same "input/typed data" is entered again.

Possible Feature Suggestion: Inline encryption, would be cool as well for say a group, channel or user2user chat...

I love the app by the way and would like to contribute / get involved as well.

Thank You, Dean :)

Initial Issue Report: https://stackoverflow.com/questions/48930326/unkown-data-in-chat-please-explain-rocket-chat-only/

@didierhk
Copy link

@gromain @rodrigok is this still in the works ?

@gromain
Copy link
Author

gromain commented Jul 29, 2018

Hi everyone,
After a break related to work, I'm planning on starting to work again on this.
Is there anyone already working on this, @didierhk @rodrigok ? Anything I should take into account?

@CLAassistant
Copy link

CLAassistant commented Dec 26, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ vcapretz
✅ gromain
❌ Romain Bazile


Romain Bazile seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support full Markdown spec: +GitLab +GitHub