-
-
Notifications
You must be signed in to change notification settings - Fork 651
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
editor: add code block language to local storage #7086
base: master
Are you sure you want to change the base?
Conversation
const cachedLanguage = | ||
config.get<(typeof Languages)[number]>("codeBlockLanguage"); | ||
|
||
useEffect(() => { | ||
if (languageDefinition) { | ||
config.set("codeBlockLanguage", languageDefinition); | ||
return; | ||
} | ||
|
||
if (!languageDefinition && cachedLanguage && cachedLanguage.filename) { | ||
updateAttributes( | ||
{ language: cachedLanguage.filename }, | ||
{ addToHistory: false, preventUpdate: false } | ||
); | ||
} | ||
}, []); |
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.
We shouldn't do this here. This will run for all codeblocks even though we just want to set the language once when a new codeblock is added. It'll also cause a flash where a codeblock is plaintext and then switches to the last used language.
Instead let's do this in code-block.ts
in the parseHTML
function of the language
attribute. Not sure if it'll work or not though so give it a try.
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.
Unfortunately, useEffect
is called for all codeblocks when the page loads. Also, I wasn't able to notice a flash when codeblock is plaintext and then switches to the local storage language. But, there is a minor bug in this PR: user won't ever be able to go back to plaintext codeblock until they delete the local storage entry. I think we should add plaintext as a language in languages.json
. What do you think?
Instead let's do this in code-block.ts in the parseHTML function of the language attribute. Not sure if it'll work or not though so give it a try.
I tried in parseHTML
of the language
attribute. The issue here is the parseHTML
doesn't get called when a new code block is added, so we can't save the code block language if user pasted a code block from VSCode for example. I'll continue trying with parseHTML
.
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.
Thanks for the comment, I realized we don't need the useEffect
at all. Please review the latest changes.
(plus I've added Plaintext in languages, let me know if we should keep this or else I can revert)
935bfab
to
064a6b3
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.
I don't think this is working. It changes the language label in the code block status bar but no highlighting actually happens.
064a6b3
to
bcb19ac
Compare
const cachedLanguage = | ||
config.get<(typeof Languages)[number]>("codeBlockLanguage"); | ||
if (!languageDefinition && cachedLanguage && cachedLanguage.filename) { | ||
this.editor.commands.updateAttributes(this.type, { | ||
language: cachedLanguage.filename | ||
}); | ||
} |
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.
Thanks again for the thorough review!
I've added the language update from local storage logic here and it works. Please let me know if this is okay. Other place this exact logic works is in the useEffect
inside the code block 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.
I just checked again and we can reuse defaultLanguage
in the Highlighter
to support this. It can be a function since the value can change.
However, the default language doesn't get stored in the code block attributes which you'll need to add support for in appendTransaction
.
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.
Done, please review
bcb19ac
to
dadcee1
Compare
* also add plaintext language option * also fix spacing issues in language selector list Signed-off-by: 01zulfi <[email protected]>
dadcee1
to
016f094
Compare
Closes #7032 #6932