-
Notifications
You must be signed in to change notification settings - Fork 61
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
Added method for direct text-to-HTML conversion #1
base: master
Are you sure you want to change the base?
Conversation
microlight.reset still exists for automatically traversing the DOM and performing in-place highlighting, but the core logic has been factored out as microlight.process for performing the same operation on a string input without side effects.
Also, it just occurred to me, I probably should've included this to begin with; here's a diff with indentation removed to make it more clear what actually changed: microlight pr diff.pdf |
@buu700 See also https://github.com/blog/967-github-secrets P.S.: |
@buu700 thanks for your suggestion. I would not merge your changeset, since I don't see a real reason for highlighting a text virtually (yet I am curious to know your thoughts on how this can be used). And besides this would increase the library size (I'm trying to keep it as minimal as possible mainly for fun). By the way: you don't need to create a DIV element which is only used for storing the HTML content. Just put it into a string and then return. |
Nice, thanks for the tip @blueimp! That feature should really be way more prominent. As far as the use case @asvd, this is how I would be using it at least: https://github.com/cyph/cyph/blob/next/shared/js/cyph/ui/directives/markdown.ts#L31. I'm not sure how common that type of usage would be, but it seems like a standard enough pattern (and addresses the complaint someone on HN had about it directly modifying the DOM). As far as the size, are you specifically referring to the increase from 2.2kb to 2.3kb, or just making the more general point that you'll want to be conservative in what you add? FWIW, the gzipped size remains at 1.4kb with or without this change. Got it, thanks for the clarification on the createElement thing. (I hadn't actually looked closely enough at the code to determine whether that was necessary; was mostly trying to make as few changes as possible.) I'll update this PR with that fixed in a bit. |
I'm just preparing a more extended version of microlight, which will support editable areas, and also include some performance tweaks. So I can consider your suggestion to be added there, but not into microlight.js which I would like to keep in a minimal state. What happens then in your project with highlighted code? Where do you put it then? Do you want to perform the highlight on server side? Because in this case the formatted text can become bigger than the library itself, as discussed in this branch on HN: https://news.ycombinator.com/item?id=11803389 |
Fair enough, I'll just maintain this version as a separate fork until the extended version is available. Ah sorry, I should clarify more specifically what we're actually doing. @cyph currently includes highlight.js in production for allowing users to send snippets of code to each other, which can only be done client-side because the context is user-generated content within a secure/E2EE chat. That currently looks pretty bad because we hadn't yet gotten around to setting up a good custom style, but Microlight out of the box turned out to be great (in addition to the reduced footprint): |
You have an editable area there, so you will definitely need an extended version :-) Why not send code not formatted, and highilght upon rendering? That would reduce the amount of data to exchange.. |
Yep, it sounds like the extended version would probably be relevant there. That is what we're doing; what made you think otherwise? The text sent over the wire is just Markdown ( |
Then it should work with the original version of microlight. You get the non-highlighted code, put it into a div with class=microlight, invoke microlight.reset() and have it highlighted. Right? |
Yeah, more or less; I would just have to change that line I linked to something like The problem is that it would then have to bypass our post-processing logic (which currently does nothing that would impact code highlighting, but could eventually) and DOMPurify sanitisation, which would be particularly problematic for us in the event that someone were to find an XSS exploit against Microlight (probably unlikely given Microlight's limited scope, but all it would take would be for one browser to interact weirdly with Microlight in some edge case). |
Just made the change you suggested earlier. Not that it really addresses your issue with accepting this PR, but the minified file size is back down to 2.2kb (gzipped is still 1.4kb). |
microlight.reset still exists for automatically traversing the DOM and performing in-place highlighting, but the core logic has been factored out as microlight.process for performing the same operation on a string input without side effects.