Skip to content
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

Use class instead of lang attribute and fix minified build #6

Merged
merged 6 commits into from
Oct 23, 2024

Conversation

tomayac
Copy link
Contributor

@tomayac tomayac commented Oct 10, 2024

  • Use class instead of lang attribute
  • Fix the minified build
  • Update dependencies

Fix minified build
Update dependencies
Copy link
Owner

@thetarnav thetarnav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why class instead of lang?
I've picked lang because it is unique and so can be easily associated with the code block.
class is ambiguous.
If you need to use a different attribute, you can change that in your renderer.

smd_min_entry.js Outdated
Comment on lines 34 to 36
// Renderer
default_renderer,
logger_renderer,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole point of the min entry is to not include the example renderers.
They exist for starting point during development by "having something on the screen" before you go and make your own.
Normal entry you use with a bundler that will tree-shake them.
The min entry is usable from a cdn and should only have whats necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, not exposing the logger_renderer then.

@tomayac
Copy link
Contributor Author

tomayac commented Oct 10, 2024

Why class instead of lang? I've picked lang because it is unique and so can be easily associated with the code block. class is ambiguous. If you need to use a different attribute, you can change that in your renderer.

If lang is your default, you possibly block lang forever if the HTML standard ever wanted to define it. Something similar has happened before with Array flattening.

@tomayac
Copy link
Contributor Author

tomayac commented Oct 10, 2024

Why class instead of lang? I've picked lang because it is unique and so can be easily associated with the code block. class is ambiguous. If you need to use a different attribute, you can change that in your renderer.

If lang is your default, you possibly block lang forever if the HTML standard ever wanted to define it. Something similar has happened before with Array flattening.

The class is also what popular syntax highlights like Prism use.

@tomayac
Copy link
Contributor Author

tomayac commented Oct 11, 2024

Actually, I'm stupid in what I said in #6 (comment). The lang attribute is already defined, and its value must be in the format defined in RFC 5646. So using something like lang="javascript" is actually wrong. The move to class makes even more sense given this background.

@tomayac
Copy link
Contributor Author

tomayac commented Oct 22, 2024

Friendly ping on this PR. Is this something you're considering? Thanks!

@thetarnav
Copy link
Owner

Right.. yes.
The artice you've sent was terrifying and eye-oppening.
I doubt this lib will block anything but I'm also not one to decide what lang attr should mean.
So while the dom renderer should just set it as class or data-lang, I don't want to rename the LANG and Attr enums. They should stay as they are. attr_to_html_attr is where the translation happens. So far I tried to align the enum with html, but that doesn't have to always be so.
Also I'm still on the fence about exporting default_renderer in the min entry. If you are gonna use a cdn, copy the code for the dom renderer, it's just there as an example. Maybe it's a bit weird to provide a library that only does half the job, but in my mind you will end up making a custom renderer anyway, once you need to change anything. (like the attr name for code blocks :))

@tomayac
Copy link
Contributor Author

tomayac commented Oct 22, 2024

Please see the current version of the PR, moved CLASS back to LANG, and only doing the translation to class in the attr_to_html_attr() function.

I think exporting the default_renderer makes a lot of sense. It certainly worked well enough for my use case (note that this is dependent on a behind-a-flag API in Chrome).

@thetarnav thetarnav merged commit 3817903 into thetarnav:main Oct 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants