Typescript Conversion #29
Conversation
|
It doesn't look like we're handling preview generation (related to EditorKit, not markdown editor): https://github.com/sn-extensions/markdown-basic/blob/master/app/components/Home.js#L163 Previews related to what is shown in the two lines under the note title in the list of notes. Here we want to escape markdown syntax. This is why we render the content even if they are just in edit mode. For example, if the user types "# hello" in the first line of the note, the preview in the list of notes in the SN interface should just say "hello". |
| preview.textContent || preview.innerText | ||
| ); | ||
| return { | ||
| html: `<div>${previewText}</div>`, |
There was a problem hiding this comment.
We should be returning either html or plain, but not both. Some editors use HTML, like the Task Editor, to generate rich previews, but this editor doesn't.
There was a problem hiding this comment.
Oh ok. Why doesn't this editor use HTML? Why can't we return both html and plain? I referenced the Token Vault editor to write this code, which uses both html and plain: https://github.com/sn-extensions/token-vault/blob/master/app/components/Home.js#L57
There was a problem hiding this comment.
Ah ok, you're right, it's possible to use both. This is because on web we support rich previews, but on mobile we need only plain reviews. But for Markdown documents, if you use # Hello in the first line, we don't want a huge header in your note preview. So that's why we want plain only.
There was a problem hiding this comment.
I don't think the huge header in the note preview is an issue here because we are only taking the textContent or innerText of the Preview HTML, so we're escaping all the HTML tags. See this reference: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_node_textcontent_innerhtml_innertext
| EditorContainer = 'editor-container', | ||
| Header = 'header', | ||
| Preview = 'preview', | ||
| SimpleMarkdown = 'simple-markdown', |
There was a problem hiding this comment.
This is poorly named (probably pre-existing). We should take this chance to rename this to something that better describes what exactly this element is.
There was a problem hiding this comment.
How do you want to rename them? We'll probably need to change them in the CSS as well
There was a problem hiding this comment.
What exactly is the element? If it's a container, it should be called that. If it's the root, it should be called that.
There was a problem hiding this comment.
What exactly do you want renamed?
There was a problem hiding this comment.
We can change simple-markdown to root or preview to preview-container, but I think editor-container and header are pretty clear
I thought you might have wanted to do the honors of deleting |
|
@ningsongshen can you review this PR and test locally in SN env? Especially see if previews are being generated correctly (as in the 2-line preview that appears in SN under the note's title) |
|
So I've just tested it (will look at the PR code a bit later), here are my comments:
|
I think that's expected.
|
|
EDIT: nevermind it is an error on my end. Note to self: use http not https This didn't happen when I last tested it but now I can't seem to import the extension 😢
|
| new ExtractTextPlugin({ filename: './dist.css', disable: false, allChunks: true}), | ||
| new MiniCssExtractPlugin({ | ||
| filename: 'dist.css', | ||
| }), | ||
| new webpack.DefinePlugin({ |
There was a problem hiding this comment.
I was updating the Bold Editor webpack config, I think that this is not needed in the later versions. (L47/64)
|
So Theodore requested some assistance finish up this PR but it's pretty big so I wanted to split it up into smaller parts that I can test and try it out to understand the changes (and for whoever is coming to work on this after me!)
|
Changes:
watchandservercommands.jsto.tsxtsconfig.jsonwebpack.config.jsFix line-height in textarea to match rendered text (1.5em). Addresses line-height issue #3var's toletandconstthisfordocument.getElementById<div>buttons to<buttons>ReactandReact-domare loaded separately, improving performanceREADME.md