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

Markdown #516

Closed
wants to merge 23 commits into from
Closed

Markdown #516

wants to merge 23 commits into from

Conversation

neuroscr
Copy link

@neuroscr neuroscr commented Sep 18, 2019

Work in progress, do not merge. Posting per Maxim's request.

Public chat works
Note to self works
conversation last message works
outgoing message works in somecases

still need to redo how private conversation work with it; going to nothing with markdown on the senders side and move the detection to the receivers side

also needs some cleanup ofc

supports the following markdown:

- *em*
- _strong_ 
- `code` 
- ```multilinecode``` 
- 4 spaces then code
- > blockquotes
- - lists

does not handle nesting of em in strong and visa versa
plan to add more from https://support.discordapp.com/hc/en-us/articles/210298617-Markdown-Text-101-Chat-Formatting-Bold-Italic-Underline-

@neuroscr neuroscr changed the title WIP: Markdown Markdown Sep 20, 2019
@neuroscr
Copy link
Author

@neuroscr
Copy link
Author

Looks like "grunt unit tests" are failing, any ideas on how to include libloki_markdown.js for those?

js/modules/loki_public_chat_api.js Show resolved Hide resolved
.use(remarkRecommended) // better cross browser support?
.use(remarkHtml); // and finally convert to HTML

window.markdoneHTMLConverter = markdown => {

Choose a reason for hiding this comment

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

markdone => markdown?

// this does allow `<b>bold example</b>` to work
.use(remarkStrip)
.use(remarkRecommended) // better cross browser support?
.use(remarkHtml); // and finally convert to HTML

Choose a reason for hiding this comment

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

Would it be possible to use window.markDownHTMLConverter directly?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I think this was done before I understood how preload.js/window worked

if (markdown) {
// DO NOT ALLOW users/servers to slip in their own custom HTML
const file = mdEngine.processSync(markdown);
text = String(file);

Choose a reason for hiding this comment

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

Is there any way we can do the markdown processing at a lower level? There seems to be some duplicated logic between public and private messages.
Why not emit the raw message here and leave the markdown handling to the message.js model?

Copy link
Author

@neuroscr neuroscr Sep 23, 2019

Choose a reason for hiding this comment

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

No lower level than when it comes in from the server TBH. Per the comment, you need to protect from a malicious server from injecting malicious HTML into the feed.

Also public chat has a separate pipeline to keep it compatible with the appdotnet ecosystem (like the irc bridges, etc). This requires no markdown in the text field. The duplicate logic is contained to the libloki_markdown library itself.

@@ -518,9 +525,12 @@ class LokiPublicChannelAPI {
}

async pollOnceForMessages() {
// mark sure send() can access this object instance
const ref = this;

Choose a reason for hiding this comment

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

This looks like a binding issue?
Where is that function send() that needs to access this instance?

Copy link
Author

Choose a reason for hiding this comment

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

remanent from a previous version, I think we can remove these 2 lines

let processedText = text;
if (hasMarkdown(text)) {
// if we detect markDown, convert to HTML
processedText = window.markdoneHTMLConverter(text);
Copy link
Author

Choose a reason for hiding this comment

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

argh I did here too!

@Mikunj
Copy link

Mikunj commented Feb 7, 2020

Going to close this for now to clean up the PRs.

@Mikunj Mikunj closed this Feb 7, 2020
@KeeJef KeeJef mentioned this pull request Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants