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

Update development branch with some changes #187

Merged
merged 15 commits into from
Feb 23, 2017

Conversation

chuajiaxuan
Copy link
Contributor

Changes include:

  1. Change all require() to import variable from filepath
  2. Update files using filepath: ".../wrapper/..." to ".../documentPackager/..." due to renaming of wrapper component to documentPackager.

Updates:

  1. Update documentPackager component to include:
    • Highlight.js plugin
    • KaTeX plugin
    • HTML XSS Filtering plugin

try {
const val = hljs.highlight(lang, str, true).value;
return `<pre class="hljs"><code>${val}</code></pre>`;
} catch (__) { return ''; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand what is this catch statement is trying to do to return a null string. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, should have named __ with error instead.
But I guess in this case, it is also better to explain what this code is trying to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is meant to catch error! Apologies for this. It has now been changed to error. Thanks!

Copy link
Contributor

@tharain tharain left a comment

Choose a reason for hiding this comment

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

Looks great to me 👍

@@ -6,7 +6,7 @@
</template>

<script>
import wrapper from '../../logic/wrapper';
import wrapper from '../../logic/documentPackager';
Copy link
Contributor

Choose a reason for hiding this comment

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

@jiayingy Should have used

import wrapper from 'src/logic/...' next time

Since our webpack is configured to resolve src automatically

try {
const val = hljs.highlight(lang, str, true).value;
return `<pre class="hljs"><code>${val}</code></pre>`;
} catch (__) { return ''; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, should have named __ with error instead.
But I guess in this case, it is also better to explain what this code is trying to do.


// Getting markdown-it to use plugins
// For KaTeX
md.use(mk);
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using a longer name for mk. e.g. mdKatex.
Otherwise, it will be hard to keep track of the plugins when there are many of them in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted! Has been changed!

whiteList.code.push('class');
whiteList.span.push('class', 'aria-hidden', 'style', 'role');

// New tags not in whiteList that are required by plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed during meeting, maybe we can use of the onIgnoreTag for future unknown tag included by plugins.

Ref: https://github.com/leizongmin/js-xss#customize-the-handler-function-for-tags-not-in-the-whitelist

@chuajiaxuan
Copy link
Contributor Author

Fixed all issues! Merging!

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.

4 participants