Uh oh!
There was an error while loading. Please reload this page.
Refactor markdown syntax highlighting. - #720
Conversation
2c2fc66 to
2e2dffeCompare| export default class DashMarkdown extends Component { | ||
| constructor(props) { | ||
| super(props); | ||
| /* eslint-disable no-new */ |
There was a problem hiding this comment.
Disabling this error as we really only do want the side-effects of creating a new Highlight object. An alternative is wrapping all of the Highlight code with a function instead of a class.
There was a problem hiding this comment.
Can you explain what's happening here?
| static hljsResolve() {} | ||
| constructor() { | ||
| if(!Highlight.isReady) { |
There was a problem hiding this comment.
Put this here to handle the case in which there are multiple dcc.Markdown elements that need syntax highlighting; we won't create new promises for each of them.
2e2dffe to
103fcb5Compare| import highlightjs from 'highlight.js/lib/highlight'; | ||
| import 'highlight.js/styles/github.css'; | ||
| import bash from 'highlight.js/lib/languages/bash'; |
There was a problem hiding this comment.
Using all of the languages that are defined in the old highlight.pack.js file. We might want to standardize this set of languages so that dash-table and dash-core-components support the same languages.
Marc-Andre-Rivet
left a comment
There was a problem hiding this comment.
Needs a changelog entry
| { | ||
| 'relative_package_path': 'highlight.pack.js', | ||
| 'namespace': 'dash_core_components' | ||
| }, |
| MarkdownHighlighter.hljs.highlightBlock(nodes[i]); | ||
| } | ||
| } else { | ||
| MarkdownHighlighter.loadhljs(); |
There was a problem hiding this comment.
I think the existing implementation does not take full advantage of the react-markdown ability to get its renderer overriden -- similarly to https://github.com/plotly/dash-core-components/pull/711/files#diff-5b56ed82805404e28302aef5147d04a2R129 it should be possible to hook up the highlight directly onto the code renderer and simplify this component / usage.
There was a problem hiding this comment.
Issue created: #724, will not be addressed as part of this PR.
dddf05a to
b679a6aCompareMarc-Andre-Rivet
commented
Dec 19, 2019
Snapshots involving dcc.Markdown are not consistent with how they were before -- they lack styling. This may be a timing issue or a new bug: https://percy.io/plotly/dash-core-components/builds/3530615/view/215175776/1280?mode=diff&browser=chrome&snapshot=215175776 Would like to see a test similar to what was done with the table. Behavior with and without highlighting override. |
shammamah-zz
commented
Dec 19, 2019
@Marc-Andre-Rivet It looks like the Percy diffs are because I am importing a stylesheet from cc @wbrgss as this might be relevant to DDK. For now I've just removed the stylesheet import and it should look the same as before. |
wbrgss
commented
Dec 19, 2019
@shammamah I'm actually theming the Does that make sense? I wonder if I have to increase the specificity of those files' rules given your change. |
shammamah-zz
commented
Dec 19, 2019
@wbrgss That makes sense! I have no problem leaving out the |
| import MarkdownHighlighter from '../utils/MarkdownHighlighter'; | ||
| import {propTypes, defaultProps} from '../components/Markdown.react'; | ||
| import '../components/css/highlight.css'; |
There was a problem hiding this comment.
If we're to keep this custom css, might as well import it dynamically when we request hljs instead of getting it dynamically with the markdown component.
| @@ -0,0 +1,18 @@ | |||
| import lazyhljs from './LazyLoader/hljs'; | |||
| import '../components/css/highlight.css'; | |||
There was a problem hiding this comment.
Better, than it the Markdown component, but I would push it even further down the stack, right down to src/third-party/highlight.js, that is, load it only if we require highlighting.
| app.layout = html.Div(dcc.Markdown(md_text)) | ||
| dash_dcc.start_server(app) | ||
| dash_dcc.driver.execute_script('window.hljs = {highlightBlock: (block) => {block.innerHTML="hljs override"}};') |
There was a problem hiding this comment.
Here we assign window.hljs but nothing in this test demonstrate that it is being used / overrides the dcc behavior.
Could we add a dash_dcc.percy_snapshot('<name of snapshot>') or similar to see that the result is what was forced.
9363cb0 to
78aaedaCompare| dash_dcc.driver.execute_script('window.hljs = {highlightBlock: (block) => {block.innerHTML="hljs override"}};') | ||
| assert dash_dcc.find_element('code').text == 'hljs override' | ||
| dash_dcc.percy_snapshot('md_code_highlight_override') |
💃
Closes#713
Related to plotly/dash-table#606