-
-
Notifications
You must be signed in to change notification settings - Fork 143
Native syntax highlighting in dcc.Markdown #562
Conversation
dash_core_components/_imports_.py
Outdated
@@ -36,14 +35,13 @@ | |||
"Loading", | |||
"Location", | |||
"LogoutButton", | |||
"Markdown", | |||
"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.
@alexcjohnson This isn't very robust, because it must be manually edited after a build. But I'm not sure how to change the auto-generated _imports_.py
. I figured it might require a change to dash-generate-components
. However, it may not be needed. The dependency, highlight.js
, is much smaller than the now-unneeded react-syntax-highlighter
.
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.
Hmm yeah that's not great... also as it is, markdown
will always be imported anyway so we haven't saved its bytes (aside from what we've already saved by swapping highlight.js
in for react-syntax-highlighter
). __init__.py
isn't auto-generated, right? Seems like we can:
- leave
_imports_.py
andMarkdown.py
where they go automatically - in
__init__.py
, filter outMarkdown
by changing the imports like:
import ._imports_ as _imports
__all__ = []
for _name in dir(_imports):
if _name != 'Markdown' and not _name.startswith('_'):
locals()[_name] = _imports[_name]
__all__.append(_name)
- add a
markdown.py
that looks exactly like yourmarkdown.__init__.py
Then, to use markdown.py
and include its dist files, you have to do from dash_core_components.markdown import Markdown
One problem with this is if people make a mistake and import dash_core_components.Markdown
it won't throw an error in python, it'll just fail to render. I suppose we could at least make the if (!window.hljs)
block throw a clear error telling you how to import the component correctly? (same for react-markdown
if we manage to pull that out too)
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 if people do dcc.markdown.Markdown('''...'''')
after the app's start (e.g. inside a callback), then the dist won't be included right?
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.
Right, that wouldn't work either. OK, so maybe it's premature to try and break this stuff out, too many ways to break it and I don't see a clean way around it. Let's file this under "should be solved by lazy loading" and put Markdown
back on par with everything else - ie a regular top-level import whose js deps are always available.
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.
👍
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.
OK, I'll do this in a more conventional way and include react-markdown
and highlight.js
in the dcc _js_dist
. If I do this, it will only add about 20kB. Before, with SyntaxHighlighter
, it added 188kB from react-syntax-highlight
, and 740kB (!) from highlight.js
. Installing highlight.js
via NPM is ~533kB 🙀.
The default build of highlight.js
is 50kB with support for "24 commonly used languages". My custom build is only 12kB; IIRC I'm only including Python, CSS, Javascript and Markdown support. You can easily create a customized highlight.js
here.
I'll suggest a highlight.js
with the following languages supported (20kB minified):
- CSS
- HTTP
- JavaScript
- Python
- Bash
- JSON
- Markdown
- HTML, XML
- SQL (?)
- "Shell Session" (
``` shell ... ```
)
Obviously we can always add more later without introducing any breaking changes. Any languages I'm missing?
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.
R
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.
Oh that's awesome! I'd just add TypeScript, and yes let's keep SQL.
At some point we can make some utilities to pick from a few bundle options for both highlight.js and plotly.js - including perhaps an option to turn both of these off entirely and let you put your own in assets
.
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.
And in fact, if react-markdown
is a single thing and isn't expected to be configurable, I'd leave it in the main dcc bundle, keep it off window
src/components/Markdown.react.js
Outdated
/** | ||
* Config options for syntax highlighting. | ||
*/ | ||
highlight_config: PropTypes.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.
This could be more specific — so far it accepts just one parameter.
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.
Good call to make this an object we can extend later, but yeah, let's make it exact
and enumerate its contents. Also:
- rather than
dark: false | true
let's dotheme: 'light' | 'dark'
so we can add more themes later if we choose. - give it a
defaultProps
entry of{}
, that'll simplify the logic a bit. (You're probably aware of this, but don't put nested values like{theme: 'light'}
in a default, that leads to unpleasant consequences when the user overrides part but not all of the prop)
Closes #559 |
{ | ||
'relative_package_path': 'highlight.pack.js', | ||
'namespace': 'dash_core_components' | ||
} |
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 we get react-markdown
into a separate run-from-window file too?
Edit: no, leave it, see #562 (comment), didn't realize that one was so small!
src/components/Markdown.react.js
Outdated
// skip highlighting if highlight.js isn't found | ||
return; | ||
} | ||
const nodes = document.querySelectorAll('pre code'); |
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.
ooh that's too greedy (and potentially slow) - can we restrict it to this 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.
Yeah, agreed. querySelectorAll
is really slow. I changed it to use a React ref callback combined with getElementsByTagName
.
src/components/Markdown.react.js
Outdated
|
||
/** | ||
* User-defined inline styles for the rendered Markdown | ||
* Legacy from SyntaxHighlighter prop `customStyles` |
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.
Not sure exactly what we used customStyle
for previously, but this one will behave a bit differently as it's applied to the container <div>
, not the <pre>
. Anyway seems reasonable to include it, but let's restrict the docstring to describing what the prop does now, and put a description of its relation to SyntaxHighlighter
in our migration guide in the docs.
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.
let's restrict the docstring to describing what the prop does now
OK, done. One thing to keep in mind is that customStyle
was one level deeper before; i.e. it was applied to the <Markdown>
child of the parent <div>
, not the parent <div>
itself, as it is now. The thing is, Markdown
isn't really a container, more of a mutator. I think there are docs examples with e.g. style={'border-left': 'grey'}
.
react-markdown will already natively wrap code blocks in `<code>` and add a language CSS class github style, e.g. with ``` python So to enable syntax highlighting, we simply iterate through the code blocks and tokenize them with highlight.js
Usage: ``` from dash_core_components.markdown import Markdown ... Markdown([ ... ]) ```
This package is >1MB minified (!) https://bundlephobia.com/[email protected]
dc499ce
to
a3b73e5
Compare
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.
Beauty 💃
not sure why Percy didn't get any snapshots but whatever... merged. |
I was using Syntax Highlighter before, I had
That I cannot really replicate anymore.
and I wrap my text between back quotes but it does not recognize c++, it works with python though. In short: |
if you scroll back enough, I think it's not in the initial 20-ish list @wbrgss customized, but we can add C++ syntax if it makes sense.
I think it's simply not supported by |
Thanks @Luvideria - I wasn't sure this got much use outside of our own docs :) Sounds like we need 2 new issues in fact:
Are there any other features |
Is there a full example available? I am trying to figure out why
isn't working. Everything looks great except no syntax highlighting on the code block, and the dark theme doesn't seem to be doing anything. Other options like adding a border box did work. This is getting rendered in a Juypter notebook which might be the issue. |
@damienrj Your code appears to work for me. You can run a demo online here, which shows that the dark mode, syntax highlighting, and callback works. Are you closing the triple quotes ( Code for the above demo (click arrow to expand)import dash
import dash_core_components as dcc
import dash_html_components as html
import dash_cytoscape as cyto
from dash.dependencies import Input, Output
import json
app = dash.Dash(__name__)
server = app.server
app.config.suppress_callback_exceptions = True
app.layout = html.Div([
dcc.Markdown(id='tapNodeData-json',highlight_config={'theme':'dark'}),
cyto.Cytoscape(
id='graph',
elements=[
{'data': {'id': 'one', 'label': 'Node 1'}, 'position': {'x': 50, 'y': 50}},
{'data': {'id': 'two', 'label': 'Node 2'}, 'position': {'x': 200, 'y': 200}},
{'data': {'source': 'one', 'target': 'two','label': 'Node 1 to 2'}}
],
),
])
@app.callback(Output('tapNodeData-json', 'children'),
[Input('graph', 'tapNodeData')])
def displayTapNodeData(data):
return ["""
#### Dash and Markdown
``` python
x = 1
if x == 1:
# indented four spaces
print("x is 1.")
` ` ` # spaces to escape backticks for GitHub
# Node Data:
## Click on a node to display the relevant metadata
""",
"""
``` python
{}
` ` `
""".format(json.dumps(data))]
if __name__ == '__main__':
app.run_server(host='0.0.0.0', debug=True) Just FYI, in the future, if you wish to get help with your syntax or usage of Dash, please open a new topic in the Community Forum. If you think you've discovered a bug, please open a new issue in this repo. These are more suitable places to discuss this kind of problem than a merged PR. |
Thanks, I will do that in the future. I mistakenly posted in here to see if there was more documentation tied to the PR. I will probably go ahead and make a PR to include an example to show the syntax highlighting for general use. Will work on chasing down the issue separately from dash now, thanks again! |
This PR removes
dcc.SyntaxHighlighter
in favor of Markdown-flavoured code syntax.Example usage (EDIT: import syntax has changed; see below for discussion):