-
Notifications
You must be signed in to change notification settings - Fork 792
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
feat(markdown): Markdown support #326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😲 This is just awesome!!! I'll take a closer look into the issues you're having tomorrow if they're not already resolved. Fantastic work!! 👏
src/components/markdown.component.js
Outdated
baseUrl: string, | ||
}; | ||
|
||
componentDidMount() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed, I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oupsy, done :)
src/components/markdown.component.js
Outdated
import React, { Component } from 'react'; | ||
import { WebView, Platform } from 'react-native'; | ||
|
||
export class MarkDown extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably be named Markdown
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to MarkdownWebview in last commit
src/components/markdown.component.js
Outdated
return `<html> | ||
<head> | ||
<meta name="viewport" content="width=device-width, initial-scale=1"> | ||
<link rel="stylesheet" href="https://machour.idk.tn/github-markdown.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing you're working on this, but for the sake of documenting it, https://machour.idk.tn/github-markdown.css
should probably be changed to a local file eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's one of the issue I was facing.
I added the file to src/assets/css for linking.
@andrewda Thanks a lot for the positive feedback! I took care of your two comments and implemented markdown rendering for issues comments, with images and emojis handling 🕺 I first was tempted to get rid of MarkdownWebview all together in favor of MarkdownHtmlview, but images in For now, Now, thinking about properly rendering the Maybe we could have gitpoint.co hosting a simple transformer and point our WebView to it whenever needed? 🤔 |
By the way, here are a few things to tweak in MarkdownHtmlview later on:
|
}, | ||
em: { | ||
...textStyle, | ||
...fonts.fontPrimaryBold, // FIXME: we need an italic font |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should italic font go here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, to render this markdown correctly. We need to bundle it like we do for other variants
|
||
if (Platform.OS === 'android') { | ||
// Android's webview won't correctly render new lines in <pre> leading | ||
// to code being rendered on a single line.. we convert those to <br />s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This is great @machour 👏 👏 Tomorrow I'll pull this down and test it 😍 Still trying to understand why MarkdownWebview and the Markdown API from GitHub will be slow 🤔 And what do you mean by a transformer? Sorry for the silly questions 😭 |
@housseindjirdeh I'm just worried about an extra API call, that's all 😄 Let me know when you check this PR, and if you're able to use a bundled css file in the Webview rather than the external one I'm using now. Couldn't achieve that on my own. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please re evaluate the switch
statement?
const mapStateToProps = state => ({ | ||
authUser: state.auth.user, | ||
}); | ||
|
||
class CommentListItemComponent extends Component { | ||
props: { | ||
comment: Object, | ||
onLinkPress: Function, | ||
// onLinkPress: Function, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! I think you missed this one
const textStyleLight = { | ||
fontSize: Platform.OS === 'ios' ? normalize(11) : normalize(12), | ||
color: colors.primaryDark, | ||
...lightFont, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is a really controlled environment but you should write this like:
const textStyleLight = {
...lightFont,
fontSize: Platform.OS === 'ios' ? normalize(11) : normalize(12),
color: colors.primaryDark
}
By doing that you'll avoid potential overwrites in the new object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely (wasn't well aware of that, thanks)
/* eslint-disable no-param-reassign */ | ||
// Convert checkboxes | ||
node.data = node.data.replace( | ||
/^\s*\[\s?\]/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this regexp into a constant in order to improve its meaning?
Something like const checkBoxes = /^.....
after doing that you can remove the comment. =]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great readability advice, thanks!
break; | ||
|
||
case 'tag': | ||
switch (node.name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I'm not a big fan of switch
statements (also they add complexity to the codebase).
What do you think about moving this into some kind of function? Or maybe use the strategy pattern to solve this?
I'd love to see a solution like that :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean something like this? http://robdodson.me/javascript-design-patterns-strategy/ (seems indeed way more readable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
Yep, that!
…On Wed, Sep 13, 2017 at 5:58 PM Mehdi Achour ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/components/markdown-htmlview.component.js
<#326 (comment)>:
> + node.data = node.data.replace(
+ /^\s*\[\s?\]/,
+ emojis.white_large_square
+ );
+ node.data = node.data.replace(/^\s*\[x\]/i, emojis.white_check_mark);
+ // Convert emojis
+ node.data = node.data.replace(/:(\w+):/g, text => {
+ const emoji = text.replace(/:/g, '');
+
+ return emojis[emoji] ? emojis[emoji] : text;
+ });
+
+ break;
+
+ case 'tag':
+ switch (node.name) {
Do you mean something like this?
http://robdodson.me/javascript-design-patterns-strategy/ (seems indeed
way more readable)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#326 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAcYUn_vpoiE0ImDU9zOLeSmQ_ZtnGRCks5siF4ugaJpZM4PT_mQ>
.
|
@alejandronanez thanks a lot, I applied your remarks in last commit. Could you tell me if this is how the strategy pattern should be implemented? Also, if you can advise: I think I'm doing something really wrong, if you look at I'm passing navigation from MakdownHtmlview as I don't have access to And for the missing |
Just pulled it down to test, all in all README's seem to look just great 👍 Issue comments are also getting there as well. However I am noticing that there seems to be some slight font differences and styles with different element nodes: as well as the size seems a little too big, we might need a little more padding for some breathing room and for some reason: there seems to be line breaks when it seems there shouldn't 🤔 So all in all some smoothing needs to be done but it's really looking solid and almost there 😍 And to help answer your questions mate:
Yep there are instances of the app where
|
@housseindjirdeh I can't access |
@housseindjirdeh I fixed the font issues and greatly simplified my work by understanding better how HtmlView works. I needed to use only I'm now struggling with two issues UI wise:
I also started supporting tables 🕺 |
Here's a big markdown chunk to showcase all features : It's very easy to make some words bold and other words italic with Markdown. You can even link to Google!
Sometimes you want numbered lists:
Sometimes you want bullet points:
Alternatively,
If you want to embed images, this is how you do it: h1h2h3h4h5h6If you'd like to quote someone, use the > character before the line:
There are many different ways to style code with GitHub's markdown. If you have inline code blocks, wrap them in backticks:
GitHub also supports something called code fencing, which allows for multiple lines without indentation:
And if you'd like to use syntax highlighting, include the language: if (isAwesome){
return true
} GitHub supports many extras in Markdown that help you reference and link to people. If you ever want to direct a comment at someone, you can prefix their name with an @ symbol: Hey @kneath — love your sweater! But I have to admit, tasks lists are my favorite:
When you include a task list in the first comment of an Issue, you will see a helpful progress bar in your list of issues. It works in Pull Requests, too! And, of course emoji! ✨ 🐫 💥 :notanemoji: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a few minor things to tweak, but overall looks absolutely fantastic!!
Ok, I think this is as far as I can go for now on Markdown support inside comments before madness kicks in 😆 Putting this for review and adoption (knowing that I still need someone to properly bundle the CSS file for MarkdownWebview which is used to render the README.md file through a WebView). Cheers ! |
let rendered = marked(md); | ||
|
||
rendered = rendered | ||
.replace(new RegExp(/<p>*>/g), '<span>') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, should be better use Just /<p>*>/g
, I think new RegExp
is not necessary :)
const todoItem = /<li>\s*\[(x|\s)?\](.*)<\/li>/g; | ||
const emojiMarkup = /:(\w+):/g; | ||
|
||
let rendered = marked(md); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this let
is not necessary, you can just
return marked(md)
.replace...
This way we can avoid using let :) What do you think about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for it. Was keeping it in a variable to be able to console.log() it ;)
src/issue/screens/issue.screen.js
Outdated
@@ -120,12 +120,21 @@ class Issue extends Component { | |||
node.attribs.class && | |||
node.attribs.class.includes('issue-link') | |||
) { | |||
navigation.navigate('Issue', { | |||
issueURL: node.attribs['data-url'].replace( | |||
let issueURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic could be transformed in a method in order to avoid let
Hey, I'm not a reviewer but I've sent some comments in the code :) I hope that's not a problem |
Welcome to the team @jouderianjr :) You should be able to submit reviews now 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lord have mercy @machour, this is one outstanding PR. Can't even express how amazing this is :O
Left some tiny tiny nitpick comments but can't find anything else I would change. Really solid job mate, this makes markdown look sooo good in the app
Once you get the minor nits that I and @jouderianjr mentioned (all really shouldn't take long at all), we'll merge this baby in
); | ||
}; | ||
|
||
export class MarkdownHtmlview extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the most minor nitpick of all nitpicks, but do you think we should name this MarkdownHtmlView
? :)
a: linkStyle, | ||
}; | ||
|
||
const stylessheet = StyleSheet.create(styles); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another minor nitpick 👀, but could we keep the variable camelcase like so: styleSheet
. Also think it might be nicer to name it styleSheet
rather than stylesSheet
since it matches the RN API
import React, { Component } from 'react'; | ||
import { WebView, Platform } from 'react-native'; | ||
|
||
export class MarkdownWebview extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here, MarkdownWebView
since RN API also camel-cases WebView
@jouderianjr your comments are really appreciated and have been taken care of. Thanks a lot! ✋ @housseindjirdeh All done. I also embedded I'm still unable to figure out how to do load a bundled CSS for iOS thought, so I'm still pointing to an external url, could you guys have a look at it ? And last but not least, there's still the problem in Android webview where you can't scroll horizontally (to read a code sample), because the swipe is intercepted by the NavigationTab.. @jouderianjr is also having this issue for his PR : #327 (comment) Can we haz disable? 😆 |
@machour Thank you sooo much matey. Unfortunately, that's something I don't think we should be disabling 😞 I'm quite used to navigating that way and I'm sure quite a few folks are the same (discussion continued in #327) Isn't it only the far left of the screen that this And are you considering replacing |
@housseindjirdeh just tried on a real Android device, no matter where I start scrolling, the swipeEnabled kicks in. The only was to scroll is to long press the If we want to keep swipeEnabled, here's a few options we might consider:
(later option is something I'm considering for another PR, to have a strong But let's not make the scroll issue a show-stopper for now, especially if we're implementing #285 soon ;) For the fonts, I wouldn't dare messig with AvenirNext :D |
@machour Nice thank you, appreciate you including 100% agreed. i'm more than happy to merge this in for now and we can open an issue to fix the scroll issue on Android in the near future ⭐ |
I looked at #106 and #202 and implemented a better support for README.md rendering in the app, by introducing a GitHub flavoured MarkDown component:
It takes care of decorating the markup html with a stylesheet, fix a weird rendering issue with Android, and renders it in a WebView, with basePath set in order to handle internal links correctly.
Having it as component mean that we can later re-use it to render
.md
files in the Code section of the app :)I'm just facing two issues if someone can advise:
github-markdown.css
is from this package: https://github.com/sindresorhus/github-markdown-css. It should be included as an asset in the app, but I couldn't do it as WebView is kind of "tricky" to stay polite.And one design concern:
If you navigate inside the WebView using a link, and hit the back arrow on top of the screen, you'll get back to the repository screen. Do we want to intercept the click instead and navigate back in history? And is letting the user navigating freely in this WebView is a good thing? 🤔