-
-
Notifications
You must be signed in to change notification settings - Fork 11
refactor, feat: support notationHightligh (close #50), feat: editing in live preview (close #19, about #44) #51
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
base: master
Are you sure you want to change the base?
Conversation
QQ2025520-133147.mp4I referred to the textarea on the https://shiki.style/ page. The principle is to stack textarea and pre together. Warning This strategy has a drawback: it is very susceptible to the influence of themes/plugins/styles, etc. Because it is necessary to ensure that the It's still the same display: |
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.
Please explain more closely what you are doing and why you are doing it. Please keep to existing coding conventions used in the plugin, please use the set-up formatter, and please keep comments to english.
// - span | ||
// - pre | ||
// - textarea | ||
const div = document.createElement('div'); el.appendChild(div); div.classList.add('obsidian-shiki-plugin') |
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.
Please use the DOM manipulation methods provided by Obsidian to make this more concise.
'spellcheck': 'false', | ||
}; | ||
Object.entries(attributes).forEach(([key, val]) => { | ||
textarea.setAttribute(key, val); |
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.
Don't add style attributes via JS. Use CSS classes.
src/main.ts
Outdated
textarea.setAttribute(key, val); | ||
}); | ||
// async part | ||
this.codeblock_getPre(language, source).then(pre => span.innerHTML = pre); |
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.
avoid .innerHTML
if possible
src/main.ts
Outdated
@@ -102,9 +111,69 @@ export default class ShikiPlugin extends Plugin { | |||
} | |||
} | |||
|
|||
const codeBlock = new CodeBlock(this, el, source, language, ctx); | |||
let option: 'pre'|'old'|'textarea' = 'textarea' // TODO as a new setting option |
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.
Please refactor this into a separate method.
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 have already made it clear at the beginning that this is not a PR that can be merged at present. All of these were written by me in "TODO".
Here it is written in todo: This will be a new option
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.
Ah. Since this is not a draft PR, I assumed you wanted this to be reviewed and merged.
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 haven't tried draft pr. I didn't know there was this function.
I thought it would be enough to explain this matter at the beginning.
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 I'm more concerned about is that here I didn't use most of the original code logic for rendering. I didn't use codeToToken but directly codeToHtml.
Was there any consideration in following the original logic?
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.
In the CM6 plugin the plugin applies the styles encoded in the tokens to existing elements.
I pressed the wrong… 😢 don't review for now. |
There are currently some problems with this PR:
This PR works:
[!code hl]
,[!code ++]
). close Update shiki version and support notationHightligh #50TODO:
from 'shiki'
replacefrom 'shiki.mjs'
) (Because of the problems with bun and npm)tab
keyI'm trying to solve them. So currently, this PR is not a state that can be merged.
close #50
close #19
close #1
about #44