-
Notifications
You must be signed in to change notification settings - Fork 37
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
fix: yaml is correctly highlighted #96
Conversation
@@ -11,6 +11,7 @@ import { | |||
} from "./deps.ts"; | |||
import { CSS, KATEX_CLASSES, KATEX_CSS } from "./style.js"; | |||
export { CSS, KATEX_CSS, Marked }; | |||
import "https://esm.sh/[email protected]/components/prism-yaml"; |
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.
Is pulling in yaml for everyone necessary? It seems like the actual fix is the additional types below. Won't that still work if the user app pulls in YAML (like it has to pull in most languages)?
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.
Sadly I think it is. The necessary thing is making sure this returns something:
Line 53 in b9f7352
const grammar = |
The only way I could do that is by importing the package. For whatever reason I couldn't find any way to invoke the loadlanguages function that prism has.
color: var(--color-prettylights-syntax-keyword); | ||
} | ||
|
||
&.punctuation { |
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.
You added key
to the options above, but didn't add a matching style. Was that intentional?
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 think the css produced purely by prism is also lacking css for this, although I could be wrong. Is it the styling looking incorrect?
Waiting on #99, so that this PR is focused purely on YAML, instead of also redoing the testing infrastructure. |
@hashrock and @crowlKats, what do you think about this one? On the positive side, it highlights yaml better than we currently do. On the negative side, it might not be right in all cases and if the user doesn't want yaml highlighting, there's no way to opt out. I'd say the positives outweigh the negatives though. |
@deer I agree positives outweigh the negatives. It would be a positive addition to the package if added, also contributes to the goal of it all working out the box. |
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.
LGTM
closes #60
Sadly prism doesn't offer an easy way to import languages, so I've just hacked it in by doing:
The testing strategy I was pursuing needed a major refactor, which I've done here. It's now significantly easier to add server cases. Additionally, you can now easily visually inspect the test cases by running
deno task server
. This currently produces the following output:I think the highlighting on the yaml case can be improved. Please let me know what I should be doing in
main.scss
to improve this.