LBRYCommunity/lbry-desktop

Fork 0

Support markdown makeup in claim description #242

Closed

longle255 wants to merge 11 commits from claim-desc-markdown-support into master

longle255 commented

2017-06-15 21:35:48 +02:00

(Migrated from github.com)

Allow user to put rich content in claim's description by using Markdown language.

Without markdown support:

With markdown

Allow user to put rich content in claim's description by using Markdown language. Without markdown support: ![Without markdown support](https://image.prntscr.com/image/hwKPPVVaQTqzSIflfE260g.png "Without markdown support") With markdown ![With markdown support](https://image.prntscr.com/image/e0Gh2zy3S3O_pbeAlf3EkA.png "With markdown support")

kauffj commented

2017-06-15 23:49:39 +02:00

(Migrated from github.com)

@longle255 I think adding markdown support is a great idea! However, I think if we're going to do it we should flesh it out a bit more:

Switch the publish description editor to support markdown output. Possibly something like https://simplemde.com/
Either disallow h1, h2 (and maybe h3?) or style them to be smaller
Handle case of invalid or poorly formed markdown when rendering (someone can theoretically put anything in the description metadata field - will ReactMarkdown handle all of this?)

Feel free to discuss with me on Slack as well, this is definitely a valuable addition!

@longle255 I think adding markdown support is a great idea! However, I think if we're going to do it we should flesh it out a bit more: - Switch the publish description editor to support markdown output. Possibly something like https://simplemde.com/ - Either disallow h1, h2 (and maybe h3?) or style them to be smaller - Handle case of invalid or poorly formed markdown when rendering (someone can theoretically put *anything* in the description metadata field - will `ReactMarkdown` handle all of this?) Feel free to discuss with me on Slack as well, this is definitely a valuable addition!

longle255 commented

2017-06-15 23:56:05 +02:00

(Migrated from github.com)

sure, that makes sense. I was going to update the description editor too. I'll see if I can address all suggestions

longle255 commented

2017-06-16 01:39:39 +02:00

(Migrated from github.com)

@kauffj I added few changes:

editor for claim's description in publish page
limit markdown type: remove heading, html block for preventing breaking layout

@kauffj I added few changes: - editor for claim's description in publish page - limit markdown type: remove heading, html block for preventing breaking layout ![Markdown editor in publish page](https://image.prntscr.com/image/h9dsG9kpQ4O2WA-vJwOgcA.png "Markdown editor in publish page")

lyoshenka commented

2017-06-16 15:15:14 +02:00

(Migrated from github.com)

Another consideration: this opens up the possibility of someone embedding javascript in the description, which is a serious security hole. Please make sure to handle this. We probably don't want CSS either.

longle255 commented

2017-06-16 15:35:12 +02:00

(Migrated from github.com)

yes that was considered too, all HTML block which could possibly include JS and CSS will not be rendered

longle255 commented

2017-06-16 15:40:56 +02:00

(Migrated from github.com)

For example if I put a markdown like this

Normal markdown
*italic*
**bold**
<b> This should not be bold </b> <script> alert("security break");</script>
another text

those HTML tag will be escaped as screenshot

For example if I put a markdown like this ``` Normal markdown *italic* **bold** <b> This should not be bold </b> <script> alert("security break");</script> another text ``` those HTML tag will be escaped as screenshot ![escaped html](https://image.prntscr.com/image/MfcyBDhlRK2--lnAEx5Tbw.png "escaped html")

longle255 commented

2017-06-16 16:29:45 +02:00

(Migrated from github.com)

@kauffj ready for review 😁

kauffj (Migrated from github.com) requested changes 2017-06-16 23:29:55 +02:00

kauffj (Migrated from github.com) left a comment

Great work! A few more suggestions.

ui/js/component/common.js

kauffj (Migrated from github.com) commented

2017-06-16 23:22:08 +02:00

This entire function terrifies me. I'm guessing the proper way to do this is to fully parse the markdown, then return the plain text.

ui/js/component/fileCard/view.jsx

kauffj (Migrated from github.com) commented

2017-06-16 23:22:51 +02:00

Why did lines switch?

ui/js/component/form.js

kauffj (Migrated from github.com) commented

2017-06-16 23:26:32 +02:00

I'm decently confident this is possible, maybe with this._element = <SimpleMDE> or this._element = SimpleMDE? Worth looking into. If it's not, this can still be cleaned up by compacting the shared properties to one declaration and passing them in with an extract operator.

I'm decently confident this is possible, maybe with `this._element = <SimpleMDE>` or `this._element = SimpleMDE`? Worth looking into. If it's not, this can still be cleaned up by compacting the shared properties to one declaration and passing them in with an extract operator.

ui/scss/component/_form-field.scss

kauffj (Migrated from github.com) commented

2017-06-16 23:28:43 +02:00

Is this !important unavoidable?

Is this `!important` unavoidable?

kauffj commented

2017-06-16 23:35:47 +02:00

(Migrated from github.com)

There's also some discussion among LBRY devs that this will require an additional metadata field.

kauffj (Migrated from github.com) approved these changes 2017-06-19 21:15:38 +02:00

kauffj commented

2017-06-21 18:08:27 +02:00

(Migrated from github.com)

This is merged into publishing branch, which will eventually be either 0.12.1 or 0.13. Either way, this is merged and considered complete, but is not going into master.

This is merged into `publishing` branch, which will eventually be either 0.12.1 or 0.13. Either way, this is merged and considered complete, but is not going into `master`.