LBRYCommunity/lbry-desktop

Fork 0

Fix markdown #1441

Merged

btzr-io merged 17 commits from markdown-fix into master

2018-05-15 17:54:32 +02:00

btzr-io commented

2018-05-07 07:10:36 +02:00

(Migrated from github.com)

Changes

Fix markdown render #1179

Test

Test for all supported features.

lbry://content-type#c2740b0ccd93fc7aae7e094dc078dcb178bd9a6c

Security test

lbry://markdown-xss#deb9947d603c63dc3a7350bd83212836b74a16cc

Todo

Security test
Filter url
Confirmation for external links https://github.com/lbryio/lbry-app/issues/1442

### Changes Fix markdown render #1179 ### Test Test for all supported features. > lbry://content-type#c2740b0ccd93fc7aae7e094dc078dcb178bd9a6c ### Security test - [Markdown's XSS Vulnerability (and how to mitigate it)](https://github.com/showdownjs/showdown/wiki/Markdown%27s-XSS-Vulnerability-(and-how-to-mitigate-it)) - [Markdown XSS Payloads](https://github.com/cujanovic/Markdown-XSS-Payloads) > lbry://markdown-xss#deb9947d603c63dc3a7350bd83212836b74a16cc ### Todo - [x] Security test - [x] Filter url - [x] Confirmation for external links https://github.com/lbryio/lbry-app/issues/1442

neb-b (Migrated from github.com) reviewed 2018-05-07 07:10:36 +02:00

tzarebczan commented

2018-05-10 23:51:55 +02:00

(Migrated from github.com)

@btzr-io - do you think we can handle lbry:// links internally, or should we have them call open.lbry.io somehow?

[*THE UNMASKING, Part 2: The \"Scam Artist\"*](lbry://theunmasking-part2-thescamartist)

Example at the bottom of lbry://@NorVegan#5cd109fb589944d1045a670dc4126e2640b0dccb/theunmasking-part1-thenarcissist

@btzr-io - do you think we can handle lbry:// links internally, or should we have them call open.lbry.io somehow? `[*THE UNMASKING, Part 2: The \"Scam Artist\"*](lbry://theunmasking-part2-thescamartist)` Example at the bottom of lbry://@NorVegan#5cd109fb589944d1045a670dc4126e2640b0dccb/theunmasking-part1-thenarcissist

tzarebczan commented

2018-05-11 00:10:02 +02:00

(Migrated from github.com)

Also, should/could we show hyperlinks on hover over? Traditional browsers have a status bar which would show the target link.

Otherwise this PR looks great to me...much better than what we had before (barely any support).

Also, should/could we show hyperlinks on hover over? Traditional browsers have a status bar which would show the target link. Otherwise this PR looks great to me...much better than what we had before (barely any support).

neb-b commented

2018-05-11 00:37:12 +02:00

(Migrated from github.com)

They should definitely be handled internally

👍 1

neb-b commented

2018-05-11 01:17:26 +02:00

(Migrated from github.com)

Finally tested this. This is awesome!! I merged the lbry-redux change, I think you just need to change url to uri for the modal to work properly. One thing, all the headers look the same to me (not a big deal). This is really cool

Finally tested this. This is awesome!! I merged the `lbry-redux` change, I think you just need to change `url` to `uri` for the modal to work properly. One thing, all the headers look the same to me (not a big deal). This is really cool <img width="513" alt="screen shot 2018-05-10 at 7 16 50 pm" src="https://user-images.githubusercontent.com/16882830/39898856-bde48108-5486-11e8-9d7e-93753c4f86ce.png">

👍 1

neb-b (Migrated from github.com) requested changes 2018-05-11 01:18:39 +02:00

neb-b (Migrated from github.com) left a comment

Just need to change url to uri

Just need to change `url` to `uri`

neb-b commented

2018-05-11 01:19:43 +02:00

(Migrated from github.com)

@jiggytom that link doesn't work for me

btzr-io commented

2018-05-11 09:44:55 +02:00

(Migrated from github.com)

Todo

Fix conflicts.
Fix Invalid claim uri (local links) breaks app. (See security test and click invalid link)

### Todo - [x] Fix conflicts. - [x] Fix Invalid claim uri (local links) breaks app. (See security test and click `invalid link`)

btzr-io commented

2018-05-11 09:49:42 +02:00

(Migrated from github.com)

@seanyesmunt I added a basic test for local links.
BTW Is there a way to validate a lbry uri?
currently the app breaks (green screen + errors) if you click an uri like: lbry:wrong#uri
Should this be handle in this PR ?

@seanyesmunt I added a basic test for local links. ~BTW Is there a way to validate a lbry uri? currently the app breaks (green screen + errors) if you click an uri like: `lbry:wrong#uri` Should this be handle in this `PR` ?~

btzr-io commented

2018-05-11 09:54:09 +02:00

(Migrated from github.com)

Navigate to invalid uri breaks app (green screen + errors)

~~@tzarebczan is this a known / valid issue ? ^^~~

> Navigate to invalid uri breaks app (green screen + errors) ~@tzarebczan is this a known / valid issue ? ^^~

btzr-io commented

2018-05-11 17:02:40 +02:00

(Migrated from github.com)

I totally forgot about lbryURI, I'll implement this ASAP 😛
https://github.com/lbryio/lbry-redux/blob/master/src/lbryURI.js

I totally forgot about `lbryURI`, I'll implement this ASAP :stuck_out_tongue: https://github.com/lbryio/lbry-redux/blob/master/src/lbryURI.js

btzr-io commented

2018-05-11 17:32:12 +02:00

(Migrated from github.com)

One thing, all the headers look the same

@seanyesmunt I guess @kauffj wanted to just keep one type of heading: h3

I'd say allow h3 and smaller and tables. https://github.com/lbryio/lbry-app/issues/1179#issuecomment-382875571

I think is better to just set all titles to the same size to have more consistency.

> One thing, all the headers look the same @seanyesmunt I guess @kauffj wanted to just keep one type of heading: `h3` > I'd say allow h3 and smaller and tables. https://github.com/lbryio/lbry-app/issues/1179#issuecomment-382875571 I think is better to just set all titles to the same size to have more consistency.

kauffj commented

2018-05-11 17:33:55 +02:00

(Migrated from github.com)

@btzr-io I think some variation in heading sizing is useful for information layout. But I agree using CSS to restrict sizing rather than restricting the tags themself is a superior solution. So maybe just size h1, h2, h3 all the same?

👍 1

btzr-io commented

2018-05-11 17:45:37 +02:00

(Migrated from github.com)

I think h1 and h2 are commonly used for titles and h3...h6 for subtitles
@kauffj maybe split them in two groups title: h1, h2 , subtitle: h3..h6?

Github markdown (example):

H1

H2

H3

H4

H5

H6

I think `h1` and `h2` are commonly used for titles and h3...h6 for subtitles @kauffj maybe split them in two groups title: `h1, h2` , subtitle: `h3..h6`? Github markdown (example): # H1 ## H2 ### H3 #### H4 ##### H5 ###### H6

btzr-io commented

2018-05-11 17:51:03 +02:00

(Migrated from github.com)

Todo

Fix heading size: https://github.com/lbryio/lbry-app/pull/1441#issuecomment-388399632
Show hyperlinks on hover over

### Todo - [x] Fix heading size: https://github.com/lbryio/lbry-app/pull/1441#issuecomment-388399632 - [x] Show hyperlinks on hover over

kauffj commented

2018-05-11 18:00:27 +02:00

(Migrated from github.com)

@btzr-io what I'm trying to avoid in both of my suggestions is rendered markup with headers that is larger than the content outside of it. Basically, I don't want people to be able to break the sizing hierarchy that their content is embedded inside of. Hopefully that made sense. As long as we're doing that, I'm not particularly attached to any one solution.

👍 1

tzarebczan (Migrated from github.com) approved these changes 2018-05-15 00:05:23 +02:00

tzarebczan (Migrated from github.com) left a comment

@btzr-io internal links look good, glad you found the validator. Hover over support also looks good!
The green screen issue is a known bug, filed under https://github.com/lbryio/lbry-app/issues/959.

@btzr-io internal links look good, glad you found the validator. Hover over support also looks good! The green screen issue is a known bug, filed under https://github.com/lbryio/lbry-app/issues/959.

neb-b commented

2018-05-15 17:54:28 +02:00

(Migrated from github.com)

This is awesome