TryQuiet / quiet

A private, p2p alternative to Slack and Discord built on Tor & IPFS
https://www.tryquiet.org
GNU General Public License v3.0
1.97k stars 85 forks source link

User can use basic markdown #1088

Closed holmesworcester closed 1 year ago

holmesworcester commented 1 year ago

Jason and I decided we should use other open source apps as inspiration for styles. For example https://github.com/mattermost/

It should look good on desktop and mobile, which means finding libraries for both React and React Native.

Related to #1025

The most important thing is that this should work and

this...
should work

Slack does not implement "#" and "##" and "------" for H1, H2, etc and I think we should also not do this.

Bold and [link](https://link.com) should work too.

josephlacey commented 1 year ago

@holmesworcester A couple questions about the scope for this feature based on the implementation in GitHub, Mattermost and Slack.


Possible options for rendering libraries.

Desktop

Mobile

holmesworcester commented 1 year ago

Someone we want to try quiet specifically requested the stuff I mention in the description: inline code, code blocks, links, and bold. We could throw in Italic and strikethrough, or quotes, if those are easy.

Preview, key bindings, and toolbar are definitely features we can tackle separately and not necessary here.

It seems like there are two approaches here: implement the features one by one, or grab some library and do what's easy with the library. The requirement that we get the above features working means we should focus on rending the markdown and not creating it... we can let the user do that themselves in a text field, and later we can implement editing and the other stuff.

We might want to see what library Mattermost uses and use that. I'm relatively agnostic as to the library. We'll need to pick one for React Native as well, because we can't just use webviews for each message in React Native, I don't think.

josephlacey commented 1 year ago

Preview, key bindings, and toolbar are definitely features we can tackle separately and not necessary here.

Agreed.

grab some library and do what's easy with the library. The requirement that we get the above features working means we should focus on rending the markdown and not creating it... we can let the user do that themselves in a text field, and later we can implement editing and the other stuff.

This second approach sounds right to me. I'll test what's out of the box with the libraries I found, and compare parity between the desktop and mobile options.

josephlacey commented 1 year ago

@holmesworcester Just an intermediate update on this. I've got a working version for desktop locally. The react-markdown library proved most compatible to our build. (I also tested react-remark, but it didn't have some of the customizations we needed.)

The implementation includes all the features of the markdown library except the blacklisted elements, namely heading tags and the horizontal rule. I also had to remove paragraph tags because it was adding them to all markdown converted messages and creating a design spacing issue.

Handling links required a little bit of work. It's not possible to pass the converted markdown to Linkify, so my working version removes that library from the TextMessage component and reimplements links with the markdown library.

My next steps are to find a React Native library that will handle the mobile implementation similarly.

josephlacey commented 1 year ago

For the mobile implementation, I went with react-native-markdown-package because it has commits within the last year and allows the configurability we need to maintain our own design and keep it synced with the desktop implementation.

I've rewritten the default styles to remove additional heading styles and to maintain the current design. One still missing parts is removing the horizontal rule rendering. I'll need to dig into this a little more to see if I can write a custom rule for rendering it.

Links also needed a reimplementation, like they did with the desktop. I removed Linkify and made the basic styling changes, but the openUrl function that's passed into the component isn't compatible with the link handling of the markdown library. The latter is written to expect a Promise when clicking a link. Instead of using our openUrl function, I've switched over to the default react-native openURL function, which the markdown library recommends. But I'm not sure what our custom function does other than opening URLs, so this change may have implications I'm not aware of.

holmesworcester commented 1 year ago

Great! For openURL I think we had our own function there because in an earlier version of the app we were catching external links and displaying a warning, similar to Discord. We have an open issue to add this back in but no immediate plans to work on it, so we can worry about this in the future.

holmesworcester commented 1 year ago

Also I'm okay with a horizontal line if it's easier to keep it than to remove it.

Though we should make it consistent.

Can you post screenshots of a complex markdown message? We'll probably want some chromatic/storybook tests to make sure we don't mess up this style.

josephlacey commented 1 year ago

Can you post screenshots of a complex markdown message? We'll probably want some chromatic/storybook tests to make sure we don't mess up this style.

Here are the screenshots of the testing input without markdown rendering.

No markdown rendering 1

No markdown rendering 2

josephlacey commented 1 year ago

Here's the desktop rendering,

Desktop markdown rendering 1

Desktop markdown rendering 2

josephlacey commented 1 year ago

Here's the mobile rendering

Mobile markdown rendering 1

Mobile markdown rendering 2

josephlacey commented 1 year ago

The differences between the libraries with their out-of-the-box rendering that I'm seeing with these testing snippets,

One other detail: it looks like current desktop rendering converts tildes to dashes.

If there's an existing style guide I can reference, I can start implementing that. Otherwise I can make some inferences from the existing style or check the style implementations from GitHub, Mattermost and Slack.

holmesworcester commented 1 year ago

The only thing that looks really bad to me here is the quote styling on desktop. Just indenting like this might work for longer form text but not within short messages. Making desktop look like mobile in this respect would be great. This is what I mean, to be clear:

image

The lack of consistency with when quotes are rendered seems like a problem too, but a somewhat less serious one. Is there an easy fix?

We should also disable the rendering of images, since that's a security issue (you could use it to get an IP, right now, or perhaps send a malicious image) and pay close attention to how this is done so that it's not possible to work around it.

I think other than that we can start with the default styling and be responsive to user requests for changes. Maybe code blocks will need some styling too; not sure.

Also, can you put some real code (a long chunk) in the code block so I can see something more realistic? It's difficult to tell now if I think that code blocks are good enough to release.

holmesworcester commented 1 year ago

Also if it's easy to style these we should standardize around what Mattermost does.

But just using the library defaults might be easier to maintain going forward and more reliable?

holmesworcester commented 1 year ago

Also we currently support in-line LaTeX formulae in messages using $$...$$. How does this play with markdown, currently? It would be good to make that part of our storybook / test messages.

MrLuxuri commented 1 year ago

Hey guys @holmesworcester @josephlacey! Coming from discord, this is how markdown is used by everyone there, maybe we can takeaway something good from here :)

holmesworcester commented 1 year ago

@MrLuxuri do people use the headings a lot? I guess I have seen those but how big a thing is it? Slack chose not to use them so I'm on the fence.

maxvonhippel commented 1 year ago

@MrLuxuri do people use the headings a lot? I guess I have seen those but how big a thing is it? Slack chose not to use them so I'm on the fence.

I don't use headings very often, but they could be useful e.g. if you have a long pinned post at the top of a channel? For instance if people were using Quiet as something more like a weird mix between Discord and Reddit, I could imagine certain communities having laundry lists of information or rules that they pin to the top of a channel, and headings would help with that. But for day-to-day communication, my opinion is that code, italics, bold, underline, strikethrough, quote blocks, and links are the most useful features.

maxvonhippel commented 1 year ago

One other thought - this might cause some unexpected issues when combined with the latex feature. For example, who wins in this scenario?

`$x_0$`

Or this one?

$`x`$

It would be worth testing a bit for strange edge cases before merging.

josephlacey commented 1 year ago

@holmesworcester Inline updates for each of these. I think these address the other comments from the initial set of screenshots except LaTeX, which I'll respond to separately.

The only thing that looks really bad to me here is the quote styling on desktop. Just indenting like this might work for longer form text but not within short messages. Making desktop look like mobile in this respect would be great. This is what I mean, to be clear:

Here's a bunch of the styling cleaned on Desktop.

Markdown-styling-on-desktop-revisions

The lack of consistency with when quotes are rendered seems like a problem too, but a somewhat less serious one. Is there an easy fix?

I've basically done the above styling clean up for mobile, but there's a current gotcha with it. More below. The short answer though is yes, we have pretty fine-grained control over the rendering of any element once the Markdown library converts them to HTML.

We should also disable the rendering of images, since that's a security issue (you could use it to get an IP, right now, or perhaps send a malicious image) and pay close attention to how this is done so that it's not possible to work around it.

This is the gotcha. With desktop rendering, this is fixed. You'll see this in the above the screenshot, but with mobile rendering, the library doesn't allow this manipulation as easily. I'm going to look at other libraries to see if another will allow this more easily.

Also, can you put some real code (a long chunk) in the code block so I can see something more realistic? It's difficult to tell now if I think that code blocks are good enough to release.

More realistic code blocks included in the above screenshot. If you want something more complex or with a particular language, let me know.

josephlacey commented 1 year ago

@holmesworcester @maxvonhippel

Also we currently support in-line LaTeX formulae in messages using $$...$$. How does this play with markdown, currently? It would be good to make that part of our storybook / test messages.

One other thought - this might cause some unexpected issues when combined with the latex feature. For example, who wins in this scenario?

`$x_0$`

Or this one?

$`x`$

It would be worth testing a bit for strange edge cases before merging.

The LaTeX implementation has precedence over Markdown. On desktop the rendering checks if LaTeX is present, and if so, handles that, and then passes the remaining parts of the message to the regular text handling. The Markdown library is wrapper around that.

These were the test cases,

LaTeX-testing-alongside-Markdown-desktop

Mobile rending though doesn't pass the non-LaTeX parts to the Markdown renderer. I need to check this.

LaTeX-testing-alongside-Markdown-mobile

holmesworcester commented 1 year ago

Great progress. If anything is thorny let's create a new issue for it and solve it separately.

holmesworcester commented 1 year ago

We could ship markdown on desktop first, if the image loading issue requires finding a different library on mobile. Markdown is designed to be backwards compatible with plaintext so it's not so bad as a temporary state of affairs and having it on desktop adds value.

holmesworcester commented 1 year ago

Also I'm persuaded by max that headings are worth allowing. Let's allow them if that's easy, and use font sizes from Discord or Mattermost.

josephlacey commented 1 year ago

@holmesworcester

We could ship markdown on desktop first, if the image loading issue requires finding a different library on mobile. Markdown is designed to be backwards compatible with plaintext so it's not so bad as a temporary state of affairs and having it on desktop adds value.

I've been working on them side by side, so the commits are entangled on the feature branch. But I've basically got it all ready for review. There's one final issue that the new library introduced. A weird text wrapping problem. Everything else though, including the custom handling of images, is working locally. I pushed a bunch of commits for that work yesterday and will hopefully have the wrapping issue fixed and new screenshots up later today.

I did get to the bottom of the mixed LaTeX and Markdown issue on mobile. There's a conditional in the message rendering that sends the message down either path, not both like on Desktop. I assume we'll push this to another issue.

Also I'm persuaded by max that headings are worth allowing. Let's allow them if that's easy, and use font sizes from Discord or Mattermost.

I've reenabled Headings.

holmesworcester commented 1 year ago

Can you post a screenshot of the wrapping problem? How bad is it?

You have a clear path to fixing it?

josephlacey commented 1 year ago

Can you post a screenshot of the wrapping problem? How bad is it?

Yeah, it's pretty bad. Seem the line with the inline code and the image line below that. No lines are wrapping.

Markdown-mobile-line-wrapping-issue

You have a clear path to fixing it?

I do. The surrounding Typography component seems to be the culprit. Still getting to the bottom of exactly what's amiss.

josephlacey commented 1 year ago

Update desktop screenshots

desktop-markdown-revisions 1 desktop-markdown-revisions 2

josephlacey commented 1 year ago

Updated mobile screenshots

mobile-markdown-revisions 1 mobile-markdown-revisions 2

josephlacey commented 1 year ago

@holmesworcester

Can you post a screenshot of the wrapping problem? How bad is it?

Yeah, it's pretty bad. Seem the line with the inline code and the image line below that. No lines are wrapping.

You have a clear path to fixing it?

I do. The surrounding Typography component seems to be the culprit. Still getting to the bottom of exactly what's amiss.

Fixes pushed. I migrated the Typography component to within a custom paragraph rendering rule.

I think that addresses all everything on the list from earlier in this ticket, but send over any changes or thoughts.

I am going to review everything in these two libraries to make sure they aren't introducing any security holes with unaudited features. Namely I saw a boolean flag for includeHTML. I tested some obvious cases to check that this didn't just render HTML, and it didn't, but I want to take a closer look.

josephlacey commented 1 year ago

@holmesworcester

I am going to review everything in these two libraries to make sure they aren't introducing any security holes with unaudited features. Namely I saw a boolean flag for includeHTML. I tested some obvious cases to check that this didn't just render HTML, and it didn't, but I want to take a closer look.

A couple of notable features.

Two general notes about both packages,

Otherwise I think this is ready for review.

holmesworcester commented 1 year ago

@josephlacey I'm sorry we've been so slow on getting this reviewed. I didn't want it to block the latest release, and this release has dragged out. Will get review on this soon and hopefully we can release very soon.

@EmiM are there likely to be interactions with your typescript work? Can you lead on getting that resolved?

holmesworcester commented 1 year ago

@josephlacey we have to fix failing tests here to get this through. Can you follow up on this as you have time? I'm moving this back to "in progress."

josephlacey commented 1 year ago

@josephlacey we have to fix failing tests here to get this through. Can you follow up on this as you have time? I'm moving this back to "in progress."

@holmesworcester I took a look at these. I think they fall into three categories. I'll order these easiest to hardest to fix

ERR! bootstrap The "bootstrap" command was removed by default in v7, and is no longer maintained. ERR! bootstrap Learn more about this change at https://lerna.js.org/docs/legacy-package-management.

holmesworcester commented 1 year ago

The solution in the link above seems to be to downgrade to a previous version. Let's do that until ESM isn't an issue here. @josephlacey you can do this or we can do this on our end.

holmesworcester commented 1 year ago

We are okay with the solution where react-markdown is mocked, also.

josephlacey commented 1 year ago

@holmesworcester

The solution in the link above seems to be to downgrade to a previous version. Let's do that until ESM isn't an issue here. @josephlacey you can do this or we can do this on our end.

We are okay with the solution where react-markdown is mocked, also.

As I thought might be the case, version 6 of the library requires React 17, but we're on React 18, downgrading isn't a possible solution. Mocking the tests is probably the quickest solution, so I can start working on that.

holmesworcester commented 1 year ago

@vinkabuki you worked on this, correct? Can you add your notes to the issue?

holmesworcester commented 1 year ago

@josephlacey an update: vinkabuki has been out for a bit but will return to this once he's back.

kingalg commented 1 year ago

Below you can find some markdowns with a comment if they are working or not. It will be divided between multiple comments as Github doesn't accept so many attachments in one comment.

Testing examples from: https://www.markdownguide.org/basic-syntax/#:~:text=To%20create%20an%20unordered%20list,to%20create%20a%20nested%20list.

  1. Headings 

Working on desktop and mobile


Heading level 1 | Heading level 1

-- | --

Heading level 2 | Heading level 2

Heading level 3 | Heading level 3

Heading level 4 | Heading level 4

Heading level 5 | Heading level 5
Heading level 6 | Heading level 6


kingalg commented 1 year ago

Line Breaks


Working on desktop but not on mobile


To create a line break or new line end a line with two or more spaces, and then type return.

Markdown Rendered Output
This is the first line.  And this is the second line. This is the first line.And this is the second line.


kingalg commented 1 year ago

Italic


Working on desktop and mobile


To italicize text, add one asterisk or underscore before and after a word or phrase. To italicize the middle of a word for emphasis, add one asterisk without spaces around the letters.

Markdown Rendered Output
Italicized text is the cat's meow. Italicized text is the cat’s meow.
Italicized text is the cat's meow. Italicized text is the cat’s meow.
Acatmeow Acatmeow


kingalg commented 1 year ago

 Paragraphs


Not working on desktop and mobile


Markdown Rendered Output
I really like using Markdown.I think I'll use it to format all of my documents from now on. I really like using Markdown.I think I'll use it to format all of my documents from now on.


kingalg commented 1 year ago

Bold and Italic

To emphasise text with bold and italics at the same time, add three asterisks or underscores before and after a word or phrase. To bold and italicize the middle of a word for emphasis, add three asterisks without spaces around the letters.

Markdown Rendered Output
This text is really important. This text is really important.
This text is really important. This text is really important.
This text is really important. This text is really important.
This text is really important. This text is really important.
This is reallyveryimportant text. This is reallyveryimportant text.


kingalg commented 1 year ago

Ordered Lists

Working on desktop and mobile


To create an ordered list, add line items with numbers followed by periods. The numbers don’t have to be in numerical order, but the list should start with the number one.

Markdown Rendered Output
1. First item2. Second item3. Third item4. Fourth item First itemSecond itemThird itemFourth item
1. First item1. Second item1. Third item1. Fourth item First itemSecond itemThird itemFourth item
1. First item8. Second item3. Third item5. Fourth item First itemSecond itemThird itemFourth item
1. First item2. Second item3. Third item    1. Indented item    2. Indented item4. Fourth item First itemSecond itemThird itemIndented itemIndented itemFourth item

kingalg commented 1 year ago

Unordered Lists

To create an unordered list, add dashes (-), asterisks (*), or plus signs (+) in front of line items. Indent one or more items to create a nested list.

Working on desktop and mobile (except for the last item in the table). 


Screenshot 2023-09-12 at 10 28 17


kingalg commented 1 year ago

Links To create a link, enclose the link text in brackets (e.g., [Duck Duck Go]) and then follow it immediately with the URL in parentheses (e.g., (https://duckduckgo.com)). My favorite search engine is Duck Duck Go.

Screenshot 2023-09-12 at 10 26 54
kingalg commented 1 year ago

Fenced Code Blocks Working on desktop and mobile

Screenshot 2023-09-12 at 10 27 28
kingalg commented 1 year ago

Version: 2.0.0-alpha.7 Comments above - working.